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

Tickets/dm 5933 #13

Merged
merged 4 commits into from Jul 8, 2016
Merged

Tickets/dm 5933 #13

merged 4 commits into from Jul 8, 2016

Conversation

parejkoj
Copy link
Collaborator

@parejkoj parejkoj commented Jul 5, 2016

Use astrometrySourceSelector to select sources.

Cleanup config fields, removing some, and moving others to the old
starSelectorConfig, to be deleted later.

Add astrometrySourceSelector and use the registry to configure it.

Pull ccdImageList construction into separate method, with commented block
containing the old starSelector in case we need to reference it in the near
future. Also wrapped that with a profiling block, to help with speed tests.

Rename assoc -> associations

Tweak minSnr to keep relative/absolute RMS thresholds happy.
@@ -21,7 +21,7 @@ namespace jointcal {
LSST_CONTROL_FIELD(sourceFluxField, std::string, "name of flux field in source catalog");

JointcalControl() :
sourceFluxField("base_CircularApertureFlux_7")
sourceFluxField("None")
Copy link

Choose a reason for hiding this comment

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

I'm not convinced this is doing what you think it is doing, or that perhaps I don't understand something about LSST_CONTROL_FIELDS. You are setting it to "None" so that a user must specify a parameter. However "None" is still a string, and thus validate() will still succeed and the program will fail later when it tries to find the None in the schema. Wouldn't it be better to leave the control field blank, so that validate the control early, and have it throw a message about needing to set it? Again, maybe I am just not following what the control field is doing and "None" has a special meaning here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm... None doesn't have special meaning, I just wanted to set it to something that was clearly not a valid field. Previously, jointcal was cout<<'ing the above value somewhere further down the line even though I set a different value for sourceFluxField later on. I made this change to ensure it wasn't pulling something else out of the schema. Validate only checks that the field isn't empty, nothing more.

I can't set it to a blank string because validate() will fail during instantiation.

I want to totally remove sourceFluxField in the future: I don't think we actually need it, but tearing it out is not going to be easy.

@parejkoj parejkoj merged commit 1db18aa into master Jul 8, 2016
@ktlim ktlim deleted the tickets/DM-5933 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