JP-3743: Make outlier detection respect weights for in-memory models

[emolter]

Resolves JP-3743

Closes #8776

This PR fixes a bug that causes the results of outlier detection to differ between the on-disk and in-memory cases any time there are weights that fall below the threshold value.

Tasks

• request a review from someone specific, to avoid making the maintainers review every PR
• add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
• Does this PR change user-facing code / API?
  • add an entry to CHANGES.rst within the relevant release section (otherwise add the no-changelog-entry-needed label to this PR)
  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • start a regression test and include a link to the running job (click here for instructions)
    • Do truth files need to be updated ("okified")?
      • after the reviewer has approved these changes, run okify_regtests to update the truth files
• if a JIRA ticket exists, make sure it is resolved properly

[emolter]

regression tests started here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.79%. Comparing base (2ab2da9) to head (f6671fd).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8777      +/-   ##
==========================================
- Coverage   60.79%   60.79%   -0.01%     
==========================================
  Files         373      373              
  Lines       38696    38701       +5     
==========================================
+ Hits        23527    23529       +2     
- Misses      15169    15172       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[emolter]

The two regtest failures are caused by an unrelated PR and match the nightly runs here.

[braingram]

LGTM. Would you queue up another regtest to test the refactoring? Once that passes (or shows only unrelated failures) I think this is good to go.

Thanks for finding and fixing the bug!

[emolter]

Would you queue up another regtest to test the refactoring? Once that passes (or shows only unrelated failures) I think this is good to go.

Will do later today, when Jenkins is free. Maybe that will also give time for one of the maintainers to add any comments they have. I doubt the regtests will fail if the unit tests do though, since I believe they all run with the default on_disk=True and the behavior there has not changed.

[melanieclarke]

This looks good to me - thanks especially for updating the unit test to catch this. But should we also add a regtest that runs with and without on_disk=True? If they take different code branches in other places, it might be a good idea to verify the results are always the same in both cases.

[emolter]

should we also add a regtest that runs with and without on_disk=True?

Good idea. The place I found for this was in the tests of the mtimage pipeline. I think this is the best choice because (1) it covers the changes to assign_mtwcs and (2) it can be parametrized easily here because the test is isolated to calwebb_image3 and doesn't run detector1 and image2 like test_nircam_image and test_miri_image do.

Slightly unrelated, but I also changed create_median to read the on_disk status directly from the ModelLibrary rather than making it a parameter to the function. I view this as a more robust and less error-prone way of doing things. Let me know what you think.

[emolter]

Another round of regression tests started here

[melanieclarke]

Good idea. The place I found for this was in the tests of the mtimage pipeline. I think this is the best choice because (1) it covers the changes to assign_mtwcs and (2) it can be parametrized easily here because the test is isolated to calwebb_image3 and doesn't run detector1 and image2 like test_nircam_image and test_miri_image do.

Great, thanks!

Slightly unrelated, but I also changed create_median to read the on_disk status directly from the ModelLibrary rather than making it a parameter to the function. I view this as a more robust and less error-prone way of doing things. Let me know what you think.

Sounds like a good idea to me.

[emolter]

regtests are clean, can this be merged now @melanieclarke ?

[melanieclarke]

Merge away!