Skip to content
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

JP-3470: Add ifu covariance scaling to extract1d #8457

Merged
merged 6 commits into from May 21, 2024

Conversation

drlaw1558
Copy link
Collaborator

@drlaw1558 drlaw1558 commented May 2, 2024

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

Closes #8083

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • 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

Copy link

codecov bot commented May 2, 2024

Codecov Report

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

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

Files Patch % Lines
jwst/extract_1d/ifu.py 13.33% 13 Missing ⚠️
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     

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

Copy link
Contributor

@jemorrison jemorrison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a straight forward addition.

@hbushouse hbushouse added this to the Build 11.0 milestone May 7, 2024
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one comment on the updated documentation.

docs/jwst/extract_1d/description.rst Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

@hbushouse
Copy link
Collaborator

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

@drlaw1558
Copy link
Collaborator Author

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.

@nden
Copy link
Collaborator

nden commented May 15, 2024

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.

@hbushouse
Copy link
Collaborator

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
Copy link
Collaborator

hbushouse commented May 17, 2024

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.

@nden
Copy link
Collaborator

nden commented May 21, 2024

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.

@hbushouse
Copy link
Collaborator

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.

@drlaw1558
Copy link
Collaborator Author

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

@hbushouse
Copy link
Collaborator

Final updates look good to me. Merging.

@hbushouse hbushouse merged commit 6c29b41 into spacetelescope:master May 21, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IFU 1d spectral extraction should account for covariance
4 participants