spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
561 stars 167 forks source link

remove unused nlow nhigh parameters from outlier detection #8595

Closed braingram closed 3 months ago

braingram commented 3 months ago

Remove the unused nlow and nhigh parameters in outlier detection.

The functionality for these was removed in https://github.com/spacetelescope/jwst/pull/5114

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 59.54%. Comparing base (23fb5cd) to head (a084f34). Report is 451 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8595 +/- ## ======================================= Coverage 59.54% 59.54% ======================================= Files 391 391 Lines 39292 39292 ======================================= Hits 23396 23396 Misses 15896 15896 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 3 months ago

@hbushouse I requested your review mainly because I thought you'd appreciate the branch name.

hbushouse commented 3 months ago

@hbushouse I requested your review mainly because I thought you'd appreciate the branch name.

Well, that's certainly simpler than using the more recent knights_who_say_ekki-ekki-ekki-ptang-zoom-boing

hbushouse commented 3 months ago

@braingram AAAAAAARGH!!!! My own fault for saying a regtest wasn't needed for this, but this is crashing all attempted operations of the level-3 pipelines for MIRI observations. This is because the MIRI team lists nhigh and nlow in their parameter reference files in CRDS, so when those params get removed from the outlier_detection_step module, the parsing of the param ref file crashes.

It will take a while to get the MIRI team to create and deliver updated ref files that no longer reference these params, so in the meantime we'll need to revert this PR. Although I think it would be best to do a sort of hybrid revert: bring back those params, but label them as being deprecated/obsolete and mention that they have no effect on the algorithm anymore (and will be completely removed in a future version).

drlaw1558 commented 3 months ago

Looks like MIRI has at least 15 param ref files that mention this (one for each imaging filter), so it's not just a single ref file update needed. I'll start that process though. Any other instruments that need to do so?

hbushouse commented 3 months ago

Looks like MIRI has at least 15 param ref files that mention this (one for each imaging filter), so it's not just a single ref file update needed. I'll start that process though. Any other instruments that need to do so?

No, I did a global "grep" of all pars ref files currently active in CRDS and the only ones in which nlow/nhigh appear are all jwst_miri_pars-outlierdetectionstep_00[02-68].asdf. None of the other instruments are referencing those params.

I would think it should be safe to create new pars ref files that exclude those params and deliver them to CRDS-OPS at your earliest convenience, since they would have no effect on B10.2 in operations (those params aren't used anyway). That way we can release the updated code into the wild whenever it's also ready to go.