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

Expand on trace #3

Merged

Conversation

melanieclarke
Copy link

Proposal to expand trace-based extraction to all use_source_posn cases.

A couple things to note:

  • I added a MIRI trace calculation, similar to the NIRSpec one you implemented.
  • I added bounding box handling to both trace calculations. This doesn't affect NIRSpec much, but it affects MIRI LRS a lot.
  • I kept the trace_offset parameter, but expanded its usage. It can now be used to tweak any aperture definition.
  • I removed the use_trace parameter, since it's redundant with use_source_posn. A trace is now computed anytime a source position is.
  • BOTS now defaults to use_source_posn=True.
  • Docs will need updating if we like these changes.

Let me know what you think - I'm eager to get something like this in because it folds in nicely with the work we're doing for optimal extraction. In particular, with these changes, I can get the optimal extraction we developed for MIRI LRS working with a toy NIRSpec PSF model very straightforwardly.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Dec 30, 2024
Copy link
Owner

@hayescr hayescr left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes @melanieclarke! I just had two small questions, otherwise I'm happy to merge this in as is.

jwst/extract_1d/extract.py Show resolved Hide resolved
jwst/extract_1d/extract.py Outdated Show resolved Hide resolved
@melanieclarke
Copy link
Author

A question for you -- do you think we should rename trace_offset to position_offset? I'm wondering if "trace" is confusing, since it's also applied to background regions.

@hayescr
Copy link
Owner

hayescr commented Jan 3, 2025

I'd be up for changing it, especially now that it's more generally applicable. position_offset sounds fine, the only other idea I had was something like aperture_offset since it will offset all apertures?

@melanieclarke
Copy link
Author

Okay, thanks, I can update that too. Unless you object, I think I like position_offset better, by analogy to the existing use_source_posn parameter. I'm not sure the background regions are consistently understood as "apertures".

@hayescr
Copy link
Owner

hayescr commented Jan 3, 2025

Nope, no strong preference, position_offset is fine with me.

@melanieclarke
Copy link
Author

Okay, done!

@hayescr
Copy link
Owner

hayescr commented Jan 3, 2025

Thanks again I'll merge this in and update add some documentation updates!

@hayescr hayescr merged commit 62d57d3 into hayescr:jp-3794_extraction_trace Jan 3, 2025
1 check passed
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