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

Improve profile tests and use a cutout when possible #354

Merged
merged 6 commits into from
May 22, 2024

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented May 21, 2024

This PR does three things:

  1. Flesh out the radial profile test with background a bit more.
  2. Add a test of finding the center of a dim star (fixes find_center fails badly if the star is faint #352)
  3. Use a cutout instead of the full data set inside CenterAndProfile for calculating the profile to improve performance (fixes CenterAndProfile is slow... #331)

@mwcraig mwcraig requested a review from JuanCab May 21, 2024 21:45
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.33%. Comparing base (36aa59e) to head (823f853).
Report is 371 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
+ Coverage   75.20%   75.33%   +0.12%     
==========================================
  Files          27       27              
  Lines        3594     3608      +14     
==========================================
+ Hits         2703     2718      +15     
+ Misses        891      890       -1     

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

@mwcraig mwcraig force-pushed the more-profile-tests branch from 49455b5 to 8e53a68 Compare May 22, 2024 15:00
Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

Mostly comments needing changes, although it does have me wondering although it has me wondering if there should be a check in the code for the cutout being too small to correctly fit the profile?

pyproject.toml Show resolved Hide resolved
stellarphot/photometry/tests/test_profiles.py Outdated Show resolved Hide resolved
stellarphot/photometry/tests/test_profiles.py Outdated Show resolved Hide resolved
stellarphot/photometry/tests/test_profiles.py Outdated Show resolved Hide resolved
stellarphot/photometry/tests/test_profiles.py Outdated Show resolved Hide resolved
stellarphot/photometry/tests/test_profiles.py Show resolved Hide resolved
stellarphot/photometry/tests/test_profiles.py Show resolved Hide resolved


def test_radial_profile_bigger_profile_than_cutout():
# Test that the cutout, used for finding the star, can be smaller than
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not following how this is a test that the cutoff can be smaller than the profile radius.

From reviewing the code of this test, it looks like you are testing that if the cutout is smaller than needed for the profile radius, the size of the cutout will be adjusted accordingly. Can you clarify the comment accordingly.

stellarphot/photometry/tests/test_profiles.py Show resolved Hide resolved
@mwcraig mwcraig requested a review from JuanCab May 22, 2024 19:15
Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

All the changes look good.

@mwcraig mwcraig merged commit af49424 into feder-observatory:main May 22, 2024
9 checks passed
@mwcraig mwcraig deleted the more-profile-tests branch May 22, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

find_center fails badly if the star is faint CenterAndProfile is slow...
3 participants