jwst icon indicating copy to clipboard operation
jwst copied to clipboard

JP-3470: Add ifu covariance scaling to extract1d

Open drlaw1558 opened this issue 1 year ago • 7 comments

Resolves JP-3470 by allowing for a parameter to be passed into extract1d accounting for IFU cube covariance.

Closes #8083

Checklist for maintainers

  • [x] added entry in CHANGES.rst within the relevant release section
  • [ ] updated or added relevant tests
  • [x] updated relevant documentation
  • [x] added relevant milestone
  • [x] added relevant label(s)
  • [x] ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR
  • [ ] Make sure the JIRA ticket is resolved properly

drlaw1558 avatar May 02 '24 17:05 drlaw1558

Codecov Report

Attention: Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 57.98%. Comparing base (781e0e0) to head (85eefe9). Report is 294 commits behind head on master.

Files Patch % Lines
jwst/extract_1d/ifu.py 13.33% 13 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8457      +/-   ##
==========================================
+ Coverage   57.93%   57.98%   +0.05%     
==========================================
  Files         387      387              
  Lines       38839    38826      -13     
==========================================
+ Hits        22502    22514      +12     
+ Misses      16337    16312      -25     

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

codecov[bot] avatar May 02 '24 18:05 codecov[bot]

regtest run started at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1424

hbushouse avatar May 07 '24 19:05 hbushouse

CI tests are failing with:

self = <LogRecord: stpipe, 20, /home/runner/work/jwst/jwst/jwst/extract_1d/ifu.py, 248, "Applying ERR covariance prefactor of %g">

    def getMessage(self):
        """
        Return the message for this LogRecord.
    
        Return the message for this LogRecord after merging any user-supplied
        arguments with the message.
        """
        msg = str(self.msg)
        if self.args:
>           msg = msg % self.args
E           TypeError: must be real number, not NoneType

hbushouse avatar May 07 '24 19:05 hbushouse

CI tests are failing with:

self = <LogRecord: stpipe, 20, /home/runner/work/jwst/jwst/jwst/extract_1d/ifu.py, 248, "Applying ERR covariance prefactor of %g">

    def getMessage(self):
        """
        Return the message for this LogRecord.
    
        Return the message for this LogRecord after merging any user-supplied
        arguments with the message.
        """
        msg = str(self.msg)
        if self.args:
>           msg = msg % self.args
E           TypeError: must be real number, not NoneType

Hm, I thought the default argument of 1.0 in the function call would be enough to deal with this, but I guess not. Added a check for value defined prior to doing anything with it.

drlaw1558 avatar May 13 '24 16:05 drlaw1558

I'd like to check the issue with the parameter default value not being picked up before we merge this, to verify it's not a larger issue.

nden avatar May 15 '24 13:05 nden

Started another regtest run at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1454

And the CI tests seem to suddenly be working OK. Not sure why ...

hbushouse avatar May 17 '24 20:05 hbushouse

Latest regtest run is clean. Only failures are unrelated (ref file updates).

So this is ready to merge as soon as @nden is OK with the earlier issue with function arguments.

hbushouse avatar May 17 '24 23:05 hbushouse

I tracked the problem with the arguments to a test jwst/extract_1d/tests/test_ifu_image_ref.py: test_ifu_3d which calls extract.do_extract_1d directly without passing a value for the parameter and so it uses the default in the do_extract1d function which is None.

So passing of parameters works as expected but if extract.do_extract_1d is to be called on its own I suggest we set a default value in the signature as well. Also remove the condition here.

nden avatar May 21 '24 12:05 nden

Alternatively, instead of removing the condition here, it could be modified to only trigger when ifu_covar_scale is not equal to 1.0. That way you avoid having the unnecessary message about it being applied when it really isn't having any effect on the data.

hbushouse avatar May 21 '24 15:05 hbushouse

I've changed that None default to 1.0, and changed the conditional to trigger when the value is not 1.0

drlaw1558 avatar May 21 '24 15:05 drlaw1558

Final updates look good to me. Merging.

hbushouse avatar May 21 '24 17:05 hbushouse