-
Notifications
You must be signed in to change notification settings - Fork 3
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-43389: Add option to disable loading diaForcedSource history #197
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments about docstrings, but otherwise I think this is ok. I do wish there was a way to test it in this code, instead of relying on ap_verify, but I'm not sure there's a good way to do that.
Raises | ||
------ | ||
RuntimeError | ||
If the Database query failed to load DiaObjects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Database query failed to load DiaObjects | |
Raised if the Database query failed to load DiaObjects. |
@@ -79,6 +82,11 @@ def run(self, exposure, apdb): | |||
exposure padded by ``pixelMargin``. DataFrame is indexed by | |||
``diaObjectId``, ``band``, ``diaSourceId`` columns. | |||
(`pandas.DataFrame`) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Returns
block above this is missing diaForcedSources
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha. That's been missing for a while... I'll add it.
doc="Load forced DiaSource history from the APDB?" | ||
"This should only be turned off for debugging purposes.", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this docstring should say something about how this was added to let us turn off forced sources for performance reasons? Maybe something like this?
doc="Load forced DiaSource history from the APDB?" | |
"This should only be turned off for debugging purposes.", | |
) | |
doc="Load forced DiaSource history from the APDB?" | |
" This should only be turned off for debugging purposes." | |
" Added to allow disabling forced sources for performance reasons during the ops rehearsal.", | |
) |
Load forced DiaSource history from the APDB? | ||
This should only be turned off for debugging purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note as above with the config docstring.
69c730a
to
408ba98
Compare
This is a debugging-only option. I elected to pass
doLoadForcedSources
in toLoadDiaCatalogsTask.run()
rather than set it as a config of the subtask, thinking that it would be easier to remove in the future this way. I would welcome alternate suggestions!