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.

Sign up for GitHub

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

Jump to bottom

DM-24277: Apply proper motion and parallax while loading refcats in Jointcal #160

Merged
merged 4 commits into from Jul 6, 2020
Merged

DM-24277: Apply proper motion and parallax while loading refcats in Jointcal #160

merged 4 commits into from Jul 6, 2020

Conversation

parejkoj
Copy link
Collaborator

No description provided.

PaulPrice
PaulPrice approved these changes Jun 19, 2020
View reviewed changes
Copy link
Contributor

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

Trivial comments below.

Can you confirm that proper motion correction can be disabled via config parameters, and that we fail over smoothly if there are no proper motion columns in the catalog?

tests/test_jointcal.py Outdated Show resolved Hide resolved
python/lsst/jointcal/jointcal.py Outdated Show resolved Hide resolved
python/lsst/jointcal/jointcal.py Outdated Show resolved Hide resolved
@parejkoj
Copy link
Collaborator Author

Can you confirm that proper motion correction can be disabled via config parameters, and that we fail over smoothly if there are no proper motion columns in the catalog?

I followed the design of meas.astrom.DirectMatchTask, where the proper motions are always applied if available, and the new jointcal Gaia astrometry defaults fail if they are not. I did not add a config option to disable them; do you see a use-case for such an option?

There is no explicit test for what happens when proper motion columns are missing from the refcat: that's all handled internally by the ReferenceObjectLoader, and I don't think it's worth duplicating any such tests here (all jointcal does is pass an epoch and expect to get the correct thing back).

@PaulPrice
Copy link
Contributor

Do we have catalogs without proper motion columns, for which this would fail?

@parejkoj
Copy link
Collaborator Author

It would only fail if you had astrometryRefObjLoader.requireProperMotion=True (which is the default now in jointcal, because the Gaia DR2 has all the PM data). It would not fail for refcats without proper motions if that was False, the PM data would just not be applied. All of that relevant logic lives in the refcat loader.

I don't actually know where our existing PS1 refcat falls, because it has some but not all of the relevant columns.

@PaulPrice
Copy link
Contributor

OK, fair enough. Thanks!

@parejkoj
Cleanup test utilities and comments
9ecff0d
@parejkoj
Apply proper motion correction when reading refcats
b2356ed 
Calculate epoch as the mean of all image dates; we could make this
configurable in the future.
Update test values: tests use Gaia, which has PM data, so the final fit
chi2 values change (most going down slightly, as expected). The
`dist_rms_absolute` values went up because the test comparison code
does not have the epoch to correct to, so that comparison will look
worse since the fit was against the corrected values.
@parejkoj parejkoj merged commit 2ce9f25 into master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulPrice PaulPrice PaulPrice approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@parejkoj @PaulPrice