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

Undo change that prevents cal files from being used by AmiAnalyze step #8448

Closed
wants to merge 1 commit into from

Conversation

rcooper295
Copy link
Contributor

This PR addresses a small issue introduced by merging #7862 which prevented cal.fits files from being used as input by the AmiAnalyze step. While the default input will be calints files, we still want to have the option of processing cal files. Changing the way the file is opened as a datamodel fixes this (opening as a generic datamodel rather than allowing datamodels to figure out what kind of model to use).

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

@rcooper295 rcooper295 requested a review from a team as a code owner April 29, 2024 18:35
@github-actions github-actions bot added the ami label Apr 29, 2024
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.75%. Comparing base (2fb073e) to head (f9ec881).
Report is 42 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8448       +/-   ##
===========================================
- Coverage   75.31%   54.75%   -20.56%     
===========================================
  Files         474      390       -84     
  Lines       38965    38866       -99     
===========================================
- Hits        29345    21281     -8064     
- Misses       9620    17585     +7965     
Flag Coverage Δ
nightly ?

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

@rcooper295
Copy link
Contributor Author

@braingram This is a very small change but I found during testing that one of the style fixes (rcooper295@22bcd01#diff-f26eee7b1441cc9775a50494797469aba0463211f4f09ef418ac407a6eea5798R78) now results in an error when trying to process 2D cal files. The datamodels.open() function correctly opens the cal file as a 2D ImageModel, which then complains when in the next step we try to expand the dimensions of the array to be able to loop over it the way we do with the 3D files. Not sure what the timeline is to get this in before the official release (and it's unlikely that people will be trying to process cal files) but since it's just a one-line change maybe it will be quick to merge it. Let me know if you need further info!

@braingram
Copy link
Collaborator

Thanks for opening the PR and describing the issue.

Any ideas as to why the change is breaking the step test?

   FAILED jwst/ami/tests/test_ami_interface.py::test_ami_analyze_step - AttributeError: No attribute 'vparity'

Do you have an example of a file that you'd like to work that is not currently working? This might make for a useful new regression test (just running that cal file).

@rcooper295
Copy link
Contributor Author

rcooper295 commented Apr 29, 2024

I can definitely provide a cal file that worked with this change. I'm not sure from looking at the test exactly why that error is happening now though, that is odd.

cal file here: /user/rcooper/Projects/NIRISS/AMI/ami3_update/ami3_regtest/jw04478001001_03102_00001_nis_cal.fits

@braingram
Copy link
Collaborator

braingram commented Apr 30, 2024

Thanks for uploading the file.

I did some testing with it and came up with an alternative approach for supporting cal files by converting the input ImageModel to a CubeModel before the dimension expansion. I also added the file you shared as a regression tests and ran it through the pipeline to produce truth files.

@rcooper295 would you give the changes in: #8451 a try with the file you shared (and any other you have on hand that you think would be helpful). I uploaded the file you shared as the input for a new regtest in the linked PR:
https://bytesalad.stsci.edu/ui/repos/tree/General/jwst-pipeline/dev/niriss/ami/jw04478001001_03102_00001_nis_cal.fits
and uploaded the 3 files produced as output when the modified ami_analyze step was run:
https://bytesalad.stsci.edu/ui/repos/tree/General/jwst-pipeline/dev/truth/test_niriss_ami3
Would you take a look at the files and let me know if there's anything unexpected? Thanks!

@braingram
Copy link
Collaborator

@rcooper295 I'm closing this in favor of #8451

Thanks for finding and describing the issue!

@braingram braingram closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants