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

Fix center profile #329

Merged
merged 9 commits into from
May 14, 2024
Merged

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented May 14, 2024

This fixes #328 by subtracting a rough background in the initial creation of the radial profile. It also fixes #330 by subtracting the background.

@mwcraig mwcraig requested a review from JuanCab May 14, 2024 03:53
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.18%. Comparing base (e5a2c01) to head (2bc733d).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   74.87%   75.18%   +0.31%     
==========================================
  Files          27       27              
  Lines        3546     3591      +45     
==========================================
+ Hits         2655     2700      +45     
  Misses        891      891              

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

@mwcraig
Copy link
Contributor Author

mwcraig commented May 14, 2024

@JuanCab -- this is actually ready for a look now

JuanCab
JuanCab previously approved these changes May 14, 2024
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.

Looks good... but check my comment about the computation of list_from_cen and make sure that is correct...

# Calculate the distance of each pixel from the center
grid_x, grid_y = np.mgrid[: self._data.shape[0], : self._data.shape[1]]
dist_from_cen = np.sqrt(
(grid_x - self._cen[1]) ** 2 + (grid_y - self._cen[0]) ** 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct, you have grid_x determined by : self._data.shape[0] but you compute the distance from center using (grid_x - self._cen[1])?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah; this is the array vs image thing.

@@ -72,6 +72,8 @@ def test_find_center_no_star():


def test_radial_profile():
# Test that both curve of growth and radial profile are correct

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a reasonable test

def test_radial_profile_with_background():
# Regression test for #328 -- image with a background level
image = make_gaussian_sources_image(SHAPE, STARS)
image = image + +make_noise_image(
Copy link
Contributor

Choose a reason for hiding this comment

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

Superfluous + sign, I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed



def test_radial_profile_with_background():
# Regression test for #328 -- image with a background level
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I believe the logic here is sound, though it took a bit for me to understand it.

@mwcraig mwcraig dismissed JuanCab’s stale review May 14, 2024 21:49

The merge-base changed after approval.

@mwcraig
Copy link
Contributor Author

mwcraig commented May 14, 2024

I fixed the "+" sign and double-checked the 1/0 index thing to confirm it is correct, going to merge.

@mwcraig mwcraig merged commit f3f1f84 into feder-observatory:main May 14, 2024
9 checks passed
@mwcraig mwcraig deleted the fix-center-profile branch May 14, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants