Raise a clear error when Camera.gain is None - #55
Merged
Conversation
`Camera.gain` defaults to `None` and, unlike `Camera.sensor`, nothing ever resolves it. `AbstractCamera.dn_to_electrons` then computed `None * a`, which silently produced a `TapData` whose `outputs` was `None` instead of raising. The failure only surfaced later in `SensorData.from_taps` as `TypeError: NoneType object is not subscriptable`, which points at the wrong place entirely. Since `msfc_ccd.fits.open` constructs a default `Camera()`, every image loaded the documented way hit this. `dn_to_electrons` now raises a `ValueError` naming the missing parameter. A measured default was considered instead, but there is no measured gain in this repository yet and the gain is tap-dependent, so a single hardcoded value would be wrong. Added tests covering `.electrons` on an image from `msfc_ccd.fits.open`, both with an explicit gain and with the default camera, since nothing exercised that path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 19 19
Lines 840 875 +35
=========================================
+ Hits 840 875 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
roytsmart
added a commit
that referenced
this pull request
Aug 19, 2026
#55 made `dn_to_electrons` raise a `ValueError` naming the missing parameter, rather than silently producing an image whose outputs are `None`. Say so, since a reader who reaches for `.electrons` will now meet that error, and note that the gain differs from tap to tap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
roytsmart
added a commit
that referenced
this pull request
Aug 19, 2026
* Expand the README and the documentation landing page Both pages described the library in one sentence and showed a single example, loading a frame and displaying it, which left the reader with no idea that the package splits a frame into its four taps, measures and removes the bias, or models the sensor. Both pages now list the public API, and add a "Key concepts" section covering what a reader has to know to use the calibration steps: an image pairs pixel values with the header that describes them, the sensor is read out through four taps which each have their own bias, the blank and overscan columns are what measures that bias, and converting to electrons needs a gain that has to be measured. The landing page gains three examples: measuring and removing the per-tap bias, reading the header, and inspecting the sensor model. The README shows the same examples with their real output. The original example keeps its position, so the figure link already in the README still resolves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Describe the error raised when the gain is missing #55 made `dn_to_electrons` raise a `ValueError` naming the missing parameter, rather than silently producing an image whose outputs are `None`. Say so, since a reader who reaches for `.electrons` will now meet that error, and note that the gain differs from tap to tap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SensorData.electronsandTapData.electronsfailed for any image opened viamsfc_ccd.fits.open():Cause
Camera.gaindefaults toNoneand, unlikeCamera.sensor(which__post_init__resolves toTeledyneCCD230()), nothing ever resolves it.AbstractCamera.dn_to_electronsthen computedNone * a, which yields aTapDatawhoseoutputsisNonerather than raising. The failure only surfaced later inSensorData.from_tapsasTypeError: 'NoneType' object is not subscriptable, pointing at the wrong place entirely.msfc_ccd.fits.openconstructs a defaultCamera(), so every image loaded the documented way hit this. Passing an explicit gain always worked.Worth noting the bug was worse than a misplaced traceback:
TapData.electronsnever raised at all, it returned aTapDatawhoseoutputswasNone, so anything consuming taps directly got silent corruption.Fix
dn_to_electronsnow raises aValueErrornaming the missing parameter before doing any arithmetic:The alternative was resolving
gainin__post_init__to a measured default for the Teledyne CCD230, mirroring howsensoris handled. That was rejected for now: there is no measured gain anywhere in this repository (the bias and dark-current reports contain none, and the only gain values in the codebase are synthetic test fixtures), andgainis documented as tap-dependent, so a single hardcoded scalar would be wrong for the per-tap case. Baking in a default is a natural follow-up once there is a measurement to bake in.Docstrings updated to match:
Camera.gainnow states there is no measured default and that it must be set beforedn_to_electrons/.electronswill work, andfits.open'scameraparameter notes that the default camera has no gain.Tests
Nothing previously exercised
.electronson an image frommsfc_ccd.fits.open— the existing image tests all build aCamerawith an explicit gain fixture.test_fits.pygainstest_open_electrons(explicit-gain camera throughfits.open, asserting both.taps.electronsand.electronscome back inelectronand are nonzero) andtest_open_electrons_default_camera(default camera raises on both). The path list is now a shared module-level_paths.AbstractTestAbstractCameragainstest_gainandtest_dn_to_electrons, which branch on whethergainisNone.TestCamerais now parametrized over bothCamera()and a camera with a per-tap gain array, so both branches are covered.Full suite passes (123 tests);
ruff checkandblack --checkare clean.🤖 Generated with Claude Code