Rename fixture recipes to *_recipe.py and add --pds3-label-method#25
Merged
Conversation
* Rename every tests/fixture_recipes/<name>.recipe.py to
<name>_recipe.py so the files are importable Python modules (the
embedded dot made them unusable as `from ... import` targets and
confused tools that key on basenames). Update the recipe-runner
glob, the in-file `python ...` doc-comments, and the
adding_an_instrument.rst example accordingly.
* Plumb a new `pds3_label_method` option through the call chain so
every `pdsparser.PdsLabel(...)` call site receives an explicit
`method=` argument:
- `PicmakerOptions.pds3_label_method: str = 'strict'` with a
validate() check against the canonical
`PDS3_LABEL_METHODS = ('strict', 'loose', 'compound', 'fast')`
tuple exported alongside the dataclass.
- `read_image_array`, `read_one_image_array`, and
`read_pds_labeled_image_array` each accept a keyword-only
`pds3_label_method='strict'` and forward it to PdsLabel.
- `_pds3_resolve_pointer` accepts the same keyword and forwards it
to its own PdsLabel call.
- `_process_one_image` reads `options.pds3_label_method` and threads
it into both call sites.
- `images_to_pics` gains a `pds3_label_method` keyword that flows
into `PicmakerOptions`.
- The CLI gains `--pds3-label-method` (and its underscore alias
`--pds3_label_method`) with `choices=PDS3_LABEL_METHODS`.
* Tighten the help-flag baseline regex to include digits
(`--[a-z0-9_-]+`) so `--16` and `--pds3-label-method` are captured
whole; regenerate tests/fixtures/.baseline-flags.txt accordingly.
* Update user_guide.rst (CLI section) and dev/pipeline.rst (validate
invariants list) with the new option. Add focused tests:
PicmakerOptions validate() accepts each value and rejects unknown;
CLI normalization defaults to 'strict', accepts each choice, and
rejects unknown via argparse.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis PR adds a configurable PDS3 label parsing method option that flows from the options layer through CLI, image I/O, and pipeline, including validation, documentation, and test coverage. Fixture recipe scripts also update their documentation to use the correct filename pattern. ChangesPDS3 Label Parsing Method Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/picmaker/io.py`:
- Around line 297-302: Validate the pds3_label_method argument at the public I/O
boundary in read_pds_labeled_image_array: check that pds3_label_method is a str
(raise TypeError if not) and that its value is one of the allowed options (e.g.,
'strict', 'lenient', etc.; raise ValueError for unknown values), and include a
clear message describing the valid choices; apply the same explicit type/value
validation to any sibling public read_pds_* functions that accept
pds3_label_method so callers get immediate, clear TypeError/ValueError rather
than downstream failures.
In `@tests/fixture_recipes/regenerate_all.py`:
- Line 2: The docstring currently mentions the pattern "sibling
`<name>_recipe.py` module." without a Sphinx cross-reference; update that
narrative to use a module role such as :mod:`<name>_recipe` so the line reads
something like "sibling :mod:`<name>_recipe` module." in the docstring of
tests/fixture_recipes/regenerate_all.py (locate the docstring containing that
phrase and replace the plain-text reference with the :mod: role).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb81ff7d-cbf7-4e4e-ad3a-cf4b47584b6f
📒 Files selected for processing (29)
docs/dev/adding_an_instrument.rstdocs/dev/pipeline.rstdocs/user_guide.rstsrc/picmaker/cli.pysrc/picmaker/io.pysrc/picmaker/options.pysrc/picmaker/pipeline.pytests/conftest.pytests/fixture_recipes/cassini_iss_recipe.pytests/fixture_recipes/corrupt_fits_recipe.pytests/fixture_recipes/corrupt_vicar_recipe.pytests/fixture_recipes/galileo_ssi_a_recipe.pytests/fixture_recipes/galileo_ssi_b_recipe.pytests/fixture_recipes/hst_acs_recipe.pytests/fixture_recipes/hst_wfc3_recipe.pytests/fixture_recipes/hst_wfpc2_recipe.pytests/fixture_recipes/malformed_numpy_recipe.pytests/fixture_recipes/malformed_pickle_recipe.pytests/fixture_recipes/nh_mvic_recipe.pytests/fixture_recipes/pds3_sample_recipe.pytests/fixture_recipes/regenerate_all.pytests/fixture_recipes/small_grayscale_recipe.pytests/fixture_recipes/small_rgb_recipe.pytests/fixture_recipes/small_tiff16_recipe.pytests/fixture_recipes/voyager_iss_recipe.pytests/fixtures/.baseline-flags.txttests/test_cli.pytests/test_cli_unit.pytests/test_pipeline_helpers.py
Comment on lines
297
to
302
| def read_pds_labeled_image_array( | ||
| filename: str | os.PathLike[str], obj: ObjectSelector = None | ||
| filename: str | os.PathLike[str], | ||
| obj: ObjectSelector = None, | ||
| *, | ||
| pds3_label_method: str = 'strict', | ||
| ) -> ReadResult | None: |
Contributor
There was a problem hiding this comment.
Validate pds3_label_method at the public I/O boundary.
read_pds_labeled_image_array accepts caller input and forwards it directly. Add explicit
type/value checks so direct library callers always get clear TypeError/ValueError
messages for invalid values.
🔧 Proposed fix
from picmaker.pil_utils import array_to_pil, pil_to_array
+from picmaker.options import PDS3_LABEL_METHODS
from picmaker.tiff16 import ReadTiff16
@@
def read_pds_labeled_image_array(
@@
) -> ReadResult | None:
@@
+ if not isinstance(pds3_label_method, str):
+ raise TypeError(
+ f'pds3_label_method must be a str, got {type(pds3_label_method).__name__}'
+ )
+ if pds3_label_method not in PDS3_LABEL_METHODS:
+ raise ValueError(
+ f'invalid pds3_label_method {pds3_label_method!r}; '
+ f'must be one of {PDS3_LABEL_METHODS}'
+ )
+
filename_str = str(filename)Also applies to: 319-334
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/picmaker/io.py` around lines 297 - 302, Validate the pds3_label_method
argument at the public I/O boundary in read_pds_labeled_image_array: check that
pds3_label_method is a str (raise TypeError if not) and that its value is one of
the allowed options (e.g., 'strict', 'lenient', etc.; raise ValueError for
unknown values), and include a clear message describing the valid choices; apply
the same explicit type/value validation to any sibling public read_pds_*
functions that accept pds3_label_method so callers get immediate, clear
TypeError/ValueError rather than downstream failures.
| @@ -1,5 +1,5 @@ | |||
| """Regenerate every binary test fixture by importing and running each | |||
| sibling `<name>.recipe.py` module. | |||
| sibling `<name>_recipe.py` module. | |||
Contributor
There was a problem hiding this comment.
Use Sphinx module cross-reference in the docstring line.
The narrative text references a module pattern without a Sphinx role. Please format this as a module reference (for example, using :mod:) to satisfy docs conventions.
As per coding guidelines, "EVERY mention of a class, method, function, module, attribute, or data constant in narrative prose MUST use appropriate Sphinx cross-reference roles (:class:, :meth:, :func:, :mod:, :attr:, :data:)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/fixture_recipes/regenerate_all.py` at line 2, The docstring currently
mentions the pattern "sibling `<name>_recipe.py` module." without a Sphinx
cross-reference; update that narrative to use a module role such as
:mod:`<name>_recipe` so the line reads something like "sibling
:mod:`<name>_recipe` module." in the docstring of
tests/fixture_recipes/regenerate_all.py (locate the docstring containing that
phrase and replace the plain-text reference with the :mod: role).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25 +/- ##
==========================================
+ Coverage 90.24% 90.26% +0.02%
==========================================
Files 19 19
Lines 1886 1891 +5
Branches 419 420 +1
==========================================
+ Hits 1702 1707 +5
Misses 96 96
Partials 88 88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
Two independent changes bundled because they touch the same areas:
Rename fixture-recipe files to drop the embedded dot. Sixteen files under
tests/fixture_recipes/were named<name>.recipe.py, which made them unimportable as Python modules and confused tools that key on file basenames. They are now<name>_recipe.py. The recipe-runner glob (regenerate_all.py), each recipe's ownpython <name>_recipe.pydocstring line, anddocs/dev/adding_an_instrument.rstare updated to match.Plumb
pds3_label_methodthrough topdsparser.PdsLabelso every call site receives an explicitmethod=argument. New canonical tuplePDS3_LABEL_METHODS = ('strict', 'loose', 'compound', 'fast')exported frompicmaker.options;PicmakerOptions.pds3_label_methodfield validated against it.Plumbing map
picmaker.optionsPDS3_LABEL_METHODSconstant +PicmakerOptions.pds3_label_method: str = 'strict'+validate()checkpicmaker.ioread_image_array,read_one_image_array,read_pds_labeled_image_arrayeach gain a keyword-onlypds3_label_method='strict'and forward it (4PdsLabel(...)call sites updated)picmaker.pipeline_pds3_resolve_pointergains the same keyword (1PdsLabel(...)call updated);_process_one_imagereadsoptions.pds3_label_methodand threads it into both call sites;images_to_picsgains apds3_label_methodkwarg that flows into the dataclasspicmaker.cli--pds3-label-method/--pds3_label_methodflag withchoices=PDS3_LABEL_METHODS; threads through_normalize_and_validateinto the option_dictTests and docs
tests/conftest.py—basic_option_dictfixture gains the new key.tests/test_cli.py— help-flag regex tightened to include digits (--[a-z0-9_-]+) so digit-bearing flags like--16and--pds3-label-methodare captured whole; baseline regenerated (now also lists--16, which the old regex was silently dropping).tests/test_cli_unit.py— new normalization tests: default is'strict', parametrized over each accepted choice, rejects unknown via argparse.tests/test_pipeline_helpers.py—PicmakerOptions.validate()accepts each documented value and rejects unknown with the expected message.docs/user_guide.rst— new CLI flag documented under selection options.docs/dev/pipeline.rst—validate()invariant list updated.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--pds3-label-methodcommand-line option to control PDS3 label parsing strictness for.LBLfiles. Supported modes:strict(default),loose,compound, andfast.Documentation
Tests