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-24495: Switch to using __file__ to reference other configs. #201

Merged
merged 2 commits into from Apr 17, 2020

Conversation

TallJimbo
Copy link
Member

config/sky.py Outdated
@@ -22,13 +22,12 @@

import os.path

from lsst.utils import getPackageDir
# The following will have to be uncommented and adapted when the
# X-talk correction will be active
#from lsst.obs.lsst.isr import SubaruIsrTask
#config.isr.retarget(SubaruIsrTask)
#config.isr.load(os.path.join(getPackageDir("obs_lsst"), "config", "isr.py"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be updated as well? I don't know the logic behind this entire commented block (which seems thoroughly out of date), but if getPackageDir will no longer be imported, this line will fail if uncommented.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure what to do with it either. obs_subaru/config/hsc/sky.py does indeed load its own ISR config override, but I don't know enough about what this task does to know why it might be okay that we don't do that right now for obs_lsst - I suspect the answer is that we never run this task on any LSST camera, and hence this whole file is a waste of time. But I don't know.

I suppose the path of least resistance is just to switch to using __file__ in the comment, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at this again and realized that this config already loads obs_lsst's custom ISR, so I'm pretty confident the entire comment is defunct. I'm just removing it.

The ISR configuration overrides are already loaded later in the config,
so we don't need a TODO reminding us to do that in the future.
@TallJimbo TallJimbo merged commit 864b34d into master Apr 17, 2020
@TallJimbo TallJimbo deleted the tickets/DM-24495 branch April 17, 2020 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants