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

Rewrite seeing profile radial profile calculation #236

Merged
merged 6 commits into from
Jan 5, 2024

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Dec 31, 2023

This rewrites the radial profile calculation from seeing_profile_functions into a class that makes use of relatively new functionality in photutils for finding radial profiles.

The radial profiling class was added to the photometry sub-package but could go somewhere else I suppose.

One subsequent change to the SeeingProfileWidget will be to move the plotting functionality into the plotting subpackage.

Cutout2D automatically handles things like the edge of an image and
allows easy translation between cutout coordinates and coordinates on the
original image.
Partially addresses feder-observatory#98 by adding a camera argument to profile finding

Fixes feder-observatory#174 by using RadialProfile and CurveOfGrowth from photutils
@mwcraig mwcraig added refactor Summer 2023 project to rewrite stellarphot internal labels Dec 31, 2023
@mwcraig mwcraig requested a review from JuanCab December 31, 2023 16:24
@codecov-commenter
Copy link

codecov-commenter commented Dec 31, 2023

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (cd69d8c) 54.44% compared to head (394c551) 55.97%.
Report is 31 commits behind head on main.

Files Patch % Lines
stellarphot/gui_tools/seeing_profile_functions.py 12.12% 29 Missing ⚠️
stellarphot/photometry/profiles.py 80.00% 22 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #236      +/-   ##
==========================================
+ Coverage   54.44%   55.97%   +1.52%     
==========================================
  Files          23       24       +1     
  Lines        2911     2973      +62     
==========================================
+ Hits         1585     1664      +79     
+ Misses       1326     1309      -17     

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

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.

The only 'showstopper' in my opinion is your assumption of an EXPOSURE keyword in the FITs file header. This, sadly, is not a safe assumption. See my comments.

with self.out:
# sub_med += med
with self.seeing_profile_plot:
r_exact, individual_counts = rad_prof.pixel_values_in_profile
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you moved these variables (r_exact and scaled_exact_counts) out of the rad_prof instance?


class CenterAndProfile:
"""
Class to dentermine center of and hold radial profile information for a star.
Copy link
Contributor

Choose a reason for hiding this comment

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

unless you are planning to really make code that bites, you want to 'determine center' not 'dentermine center' :)

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.

The only 'showstopper' in my opinion is your assumption of an EXPOSURE keyword in the FITs file header. This, sadly, is not a safe assumption. See my comments.

@mwcraig mwcraig mentioned this pull request Jan 4, 2024
3 tasks
@mwcraig mwcraig linked an issue Jan 4, 2024 that may be closed by this pull request
@mwcraig mwcraig requested a review from JuanCab January 4, 2024 00:54
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 needs better documentation, but looks good.

@mwcraig mwcraig requested a review from JuanCab January 5, 2024 15:09
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.

OK, looks good and ready for merging.

@mwcraig mwcraig merged commit 85a425a into feder-observatory:main Jan 5, 2024
8 checks passed
@mwcraig mwcraig deleted the rewrite-seeing-profile branch January 5, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal refactor Summer 2023 project to rewrite stellarphot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite comparison functions to use new RadialProfile from photutils
3 participants