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-3444: update tso photometry to use ApertureStats #8672

Merged

Conversation

jemorrison
Copy link
Collaborator

@jemorrison jemorrison commented Jul 24, 2024

Resolves JP-3444

Closes #8019

This PR addresses when there are nans in the aperture for determining tso photometry. The resulting
ecsv file contained NaN values. This update uses the photutils function ApertureStats that screens
for Nan values.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

@jemorrison jemorrison added this to the Build 11.1 milestone Jul 24, 2024
@jemorrison jemorrison requested a review from a team as a code owner July 24, 2024 19:32
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.36%. Comparing base (155408f) to head (54015b8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8672   +/-   ##
=======================================
  Coverage   60.36%   60.36%           
=======================================
  Files         372      372           
  Lines       38326    38326           
=======================================
  Hits        23137    23137           
  Misses      15189    15189           

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

Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

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

Looks fine to me so far. It's a bit hard to evaluate what this change does without some kind of diagnostic plot, e.g., a light curve before and after this PR (plus all of Melanie's changes to NaN handling)

jwst/tso_photometry/tso_photometry.py Show resolved Hide resolved
@jemorrison jemorrison requested a review from tapastro July 26, 2024 15:32
@melanieclarke melanieclarke changed the title update tso photometry to use ApertureStats JP-3444: update tso photometry to use ApertureStats Jul 26, 2024
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

This looks good to me, other than 1 small typo in the change log. We should confirm that they definitely want to ignore NaNs in sources, before we merge, and I agree we should document the choice somwhere.

CHANGES.rst Outdated Show resolved Hide resolved
@melanieclarke
Copy link
Collaborator

We should confirm that they definitely want to ignore NaNs in sources, before we merge

Just following up - this is now confirmed, in the JP ticket.

@jemorrison
Copy link
Collaborator Author

@melanieclarke Could you look at the statement I put in the documentation. Does that sound ok ?

@jemorrison jemorrison force-pushed the JP3444_tso_photomety branch from b0067fa to 3cb6627 Compare August 5, 2024 19:37
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

A couple quick typos to fix, but otherwise, I think this is ready.

docs/jwst/tso_photometry/description.rst Outdated Show resolved Hide resolved
@melanieclarke melanieclarke merged commit be80530 into spacetelescope:master Aug 6, 2024
24 of 27 checks passed
@jemorrison jemorrison deleted the JP3444_tso_photomety branch November 19, 2024 15:24
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.

Handling of NaNs in TSO photometry
4 participants