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-28233: Fix refcat position error units #167

Merged
merged 1 commit into from Jan 7, 2021
Merged

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Jan 6, 2021

No description provided.

@parejkoj
Copy link
Collaborator

parejkoj commented Jan 6, 2021

Please merge the two commits so that we don't have a non-passing commit (the test changes go with the change in the uncertainty units).

Copy link
Collaborator

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

That the dist_rms_relative increased for some tests is surprising to me. As we talked about it on the phone, that might still be ok with more constraining refcat uncertainties given that different outliers were also rejected. This brings up a good point as to whether I should have a different outlier rejection threshold for RefStars vs. MeasuredStars (we only want to reject refstars if they're clearly bad, i.e. incorrect matches).

The HSC values actually do just what I'd expect: higher final chi2 and not all that different ndof. It's unfortunate that the CFHT data didn't behave similarly, but I'm not sure that really means anything.

The big change to the DECam test doesn't surprise me: the data really is just bad (see the docstring of JointcalTestDECAM), and I've seen that jump all over with small changes.

None of my comments is really actionable here, so I think we're ok to merge this.

@@ -158,6 +158,9 @@ def test_jointcalTask_2_visits_constrainedAstrometry_no_rank_update(self):
"""Demonstrate that skipping the rank update doesn't substantially affect astrometry.
"""
relative_error, metrics = self.setup_jointcalTask_2_visits_constrainedAstrometry()
metrics['astrometry_final_chi2'] = 1069.348
metrics['astrometry_final_ndof'] = 1900
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one concerns me, but looking at the logs doesn't show anything particularly amiss (rank update version took two "outer loops" vs. one for no rank update but that doesn't necessarily mean anything). I would expect for well formed (nearly linear in the fit parameters) input data that there is almost no difference with that setting turned on. With no rank update the refcat relative and internal "absolute" astrometry is very slightly better; I don't know if that is telling us anything useful on this small amount of input data.

I wish I had a way to compute or visualize just how close to "near linear" the chi2 space really is. I'm sure there are tools for that, but I don't have them.

@erykoff erykoff merged commit cff38f6 into master Jan 7, 2021
@erykoff erykoff deleted the tickets/DM-28233 branch January 7, 2021 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants