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

DM-37053: Add AMx/AFx/ADx astrometry metrics #106

Merged
merged 1 commit into from Jun 8, 2023
Merged

Conversation

cmsaunders
Copy link
Contributor

No description provided.

@cmsaunders cmsaunders marked this pull request as ready for review May 17, 2023 21:50
@cmsaunders cmsaunders force-pushed the tickets/DM-37053 branch 2 times, most recently from 95e457b to 354bf60 Compare May 23, 2023 15:12
@cmsaunders
Copy link
Contributor Author

I want to point out that the first two commits for this PR result in AMx, ADx, and AFx metrics consistent with the current implementation in faro. The third commit modifies ADx and AFx as discussed on the Jira ticket. After the review is done on this PR, I will squash the commits.

@jeffcarlin jeffcarlin self-requested a review May 31, 2023 22:58
@jeffcarlin
Copy link
Collaborator

Looks great, Clare! A few minor comments, but overall the ticket looks good to merge.

See the inline comment re: selecting on mags between 17-21.5 -- seems unnecessary since we're already doing a S/N selection?

I had a slight concern that the default +/-1 arcmin annulus width might be too large. But it's probably fine, and it's configurable so can be changed if desired.

In a test run I did on RC2, some of the plots (histograms) didn't look great. The existence of large outliers made the auto-scaling select a large binsize and too-broad x-axis range. Not specific to this ticket, but something to consider for histograms in analysis_tools generally?

@cmsaunders
Copy link
Contributor Author

Thanks for the review, @jeffcarlin! I tried to stick as close as possible to what is currently done in faro, so that is why there are both S/N and magnitude cuts. The annulus width also is the same as in faro. We can think about changing them, but for the time being, I want to make this as close as possible to what is done in faro for continuity.

For the histogram appearance, I will add that to DM-39163, because I think it will be easiest to do all the plotting changes together.

@cmsaunders cmsaunders merged commit 78f4769 into main Jun 8, 2023
7 checks passed
@cmsaunders cmsaunders deleted the tickets/DM-37053 branch June 8, 2023 00:21
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.

None yet

2 participants