-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deprecate preserve_comments
fix spec parsing for inline comments with closing parenthesis
#107
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #107 +/- ##
==========================================
+ Coverage 54.71% 54.80% +0.08%
==========================================
Files 25 25
Lines 3010 3020 +10
==========================================
+ Hits 1647 1655 +8
- Misses 1363 1365 +2 ☔ View full report in Codecov by Sentry. |
40754d2
to
6d2de2c
Compare
preserve_comments
fix spec parsing for inline comments with closing parenthesis
docs failure should be fixed with: #109 |
Adding @hbushouse for a JWST review Adding @WilliamJamieson for a Roman review
|
this argument appears to have no effect other than breaking specs with comments that contain a closing parenthesis
e7879ec
to
09ba7a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK to me. I'm just wondering if removing preserve_comments
will have any effect on what you get when just listing the params for a given step or pipeline. For example, right now executing strun -h jump
gives you the big list of outputs:
--min_sat_area minimum required area for the central saturation of snowballs
--min_jump_area minimum area to trigger large events processing
--expand_factor The expansion factor for the enclosing circles or ellipses
--use_ellipses deprecated
--sat_required_snowball
Require the center of snowballs to be saturated
--min_sat_radius_extend
The min radius of the sat core to trigger the extension of the core
--sat_expand Number of pixels to add to the radius of the saturated core of snowballs
--edge_size Size of region on the edges of NIR detectors where a sat core is not required
--find_showers Turn on shower flagging for MIRI
--extend_snr_threshold
The SNR minimum for detection of extended showers in MIRI
--extend_min_area Min area of emission after convolution for the detection of showers
--extend_inner_radius
Inner radius of the ring_2D_kernel used for convolution
--extend_outer_radius
Outer radius of the ring_2D_Kernel used for convolution
where the comments on each param are clearly displayed. Will those go away with this change?
Thanks for taking a look and the questions. The argument parsing and help output appears unchanged with this PR. Running The spec is parsed using Line 90 in 1ade408
|
Thanks @WilliamJamieson and @hbushouse for the reviews. |
Fixes #105
The
preserve_comments
argument ends up mapping to the_inspec
argument forConfigObj
. Setting this appears to break parsing of spec lines that contain inline comments with a closing parenthesis. Thepreserve_comments
argument also appears to have no effect on parsing comments (aside from introducing the bug above).This PR deprecates
preserve_comments
and removes internal usage of the_inspec
argument. This should fix parsing inline comments for closing parenthesis (this is tested in the included test) and have no effect on comment parsing (also included in the test).Doing a quick search through romancal and jwst I see no usage of the
preserve_comments
argument so I don't expect either of those packages to require updates. Hopefully regression tests (to be run below) will reveal if there are any issues with these changes.