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

Include interval ends for sharpless, roundness, and peakmax #1978

Merged
merged 5 commits into from
Jan 3, 2025

Conversation

mcara
Copy link
Contributor

@mcara mcara commented Dec 30, 2024

Closes #1977

This PR modifies selection of sources by shapness, roundness, and peakmax to include the interval ends, i.e., that sources that have roundness 0.0 are selected instead of being rejected when sharplo=0.0.

@mcara mcara force-pushed the include-ends branch 2 times, most recently from b65a5d5 to 413d523 Compare December 30, 2024 18:23
@larrybradley
Copy link
Member

@mcara Do you recall what the old IRAF routines use? This is a breaking change, but I suppose could be classified as a bug.

@mcara
Copy link
Contributor Author

mcara commented Jan 3, 2025

Sorry, I replied in the issue: #1977 (comment)

@mcara
Copy link
Contributor Author

mcara commented Jan 3, 2025

This is a breaking change...

This could break some idealized tests, just like the one I am using in #1977 but in practice, what is the chance to have a perfectly round star (or a star with ellipticity precisely 2) in real observations? So, I believe this PR will have a minor effect, except for helping avoid this kind of tricks: https://github.com/spacetelescope/jwst/blob/6d0c2d50c72d32f9a16bf30264d3528bff53e36e/jwst/tweakreg/tests/test_tweakreg.py#L99 which seem unnatural given that the lower limit for roundness is 0 (it cannot be negative in the original IRAF code) yet I have to set this limit to a negative value in order to include 0.

Copy link
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

LGTM. The old IRAF tools also allowed values at the limits.

Could you also please add a test for DAOStarFinder?

CHANGES.rst Outdated Show resolved Hide resolved
@larrybradley
Copy link
Member

This also needs to be rebased due to a conflict in the changelog.

@larrybradley larrybradley added this to the 2.1.0 milestone Jan 3, 2025
@mcara mcara force-pushed the include-ends branch 2 times, most recently from b32695b to 3e44685 Compare January 3, 2025 17:12
Copy link
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

Thanks, @mcara. The test failure is unrelated to this PR.

@larrybradley larrybradley merged commit 715eae0 into astropy:main Jan 3, 2025
18 of 21 checks passed
@larrybradley larrybradley mentioned this pull request Jan 4, 2025
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.

Sources that match sharplo(hi) and roundlo(hi) limits should be included
2 participants