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-12602 fitting bugfixes and other cleanups #60

Merged
merged 10 commits into from Nov 16, 2017
Merged

Conversation

parejkoj
Copy link
Collaborator

This is a bit of a grab-bag of changes, but it was spurred by some changes I needed for DM-11785, and some bugs in the fitting code discovered by @boutigny and @PierreAstier .

Jointcal now raises an exception if there's a CHOLMOD error, instead of printing a message and continuing on. Making that error more obvious seems like the right approach.

Copy link

@morriscb morriscb 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 overall other than maybe adding a comment about the numbers in your tests.

'astrometryFinalChi2': 1241.6,
'astrometryFinalNdof': 2640,
'astrometryFinalChi2': 1376.24,
'astrometryFinalNdof': 2630,

Choose a reason for hiding this comment

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

Do you have a comment explaining the origins of these numbers anywhere? Are they just what is expected from the test dataset you are running?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're all empirical: whatever the value was when I got that test to pass. They're not really something that could be pre-computed in advance (at least, not without significant difficulty). So, in that sense they function more as regression tests.

This has been a sticking point for a while: I'll add a note about them to the package readme.

@@ -79,7 +79,7 @@ def test_jointcalTask_2_visits(self):
'astrometryFinalChi2': None,
'astrometryFinalNdof': 4306,
'photometryFinalChi2': None,
'photometryFinalNdof': 2391,
'photometryFinalNdof': 2333,

Choose a reason for hiding this comment

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

Ditto, with these numbers.

@parejkoj parejkoj merged commit 2cf6463 into master Nov 16, 2017
@ktlim ktlim deleted the tickets/DM-12602 branch August 25, 2018 06:44
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