DM-27255: Merge generic and HSC conf overrides in obs_subaru #329
Conversation
leeskelvin
force-pushed
the
tickets/DM-27255
branch
5 times, most recently
from
337e002
to
0ef298e
Compare
leeskelvin
force-pushed
the
tickets/DM-27255
branch
from
0ef298e
to
9d7a611
Compare
| @@ -36,7 +36,7 @@ class ColortermOverrideTestCase(unittest.TestCase): | |||
|
|
|||
| def setUp(self): | |||
There was a problem hiding this comment.
To be consistent with your other updates, should import os -- > import os.path?
Also, I can't mark the files as they weren't touched here, but it looks like the same can be done in config/datasetIngest-gen3.py and the import os can be removed in configs/cmodel.py .
There was a problem hiding this comment.
Modified tests/test_colorterms.py, config/datasetIngest-gen3.py and configs/cmodel.py as you suggest - thanks for the spot!
| 'N515': 20892961308.54041, | ||
| 'N816': 15848931924.611174, | ||
| 'N921': 19054607179.632523, | ||
| } |
There was a problem hiding this comment.
Not for this ticket, but this reminds me that I've seen warnings when processing i2/r2 filter data that we don't have this set for them. Looks like we are also missing entries for a couple of narrow-band filters: I945, N387, N400, N468, N527, N656, N718, N926, N973, & N1010. @PaulPrice, do you know if HSC has computed these? The default is probably fine for the i2/r2 (and I'm not sure we have any data for many of the narrow filters), but adding something here would have the benefit of suppressing a warning 😉
There was a problem hiding this comment.
This is a good idea, and should be addressed in a future ticket following this config merge operation.
| """ | ||
| HSC-specific overrides for FgcmBuildStars | ||
| """ | ||
|
|
There was a problem hiding this comment.
Why did this get removed? (no big deal, just wondering as in other files you just updated the Subaru part)
There was a problem hiding this comment.
These comments seemed redundant following the updates on this ticket, so I removed them where I noticed them. I wouldn't object to keeping them in however, or searching for them all and removing them all either. Keen to hear your thoughts.
There was a problem hiding this comment.
I’m a big fan of consistency, but that’s not on you as the configs are a total mix ‘n match. You did make some deliberate choices to just update Subaru -> HSC in other files, so I was wondering if there was a specific reason to do that sometimes, but delete others. Really not a big deal though...!!
There was a problem hiding this comment.
I agree. To be consistent, remaining vestigial references to these being HSC-specific config overrides have been removed.
| """ | ||
| HSC-specific overrides for FgcmBuildStarsTable | ||
| """ | ||
|
|
There was a problem hiding this comment.
As above, these comments are now largely redundant, but keen to hear your thoughts.
There was a problem hiding this comment.
All now removed.
| """ | ||
| HSC-specific overrides for FgcmFitCycle | ||
| """ | ||
|
|
There was a problem hiding this comment.
All now removed.
| """ | ||
| HSC-specific overrides for FgcmMakeLut | ||
| """ | ||
|
|
There was a problem hiding this comment.
All now removed.
| @@ -0,0 +1 @@ | |||
| config.cycleNumber = 5 | |||
There was a problem hiding this comment.
It tripped me up that this wasn't just a file rename!
There was a problem hiding this comment.
Right, this is a completely new file which did exist in 'hsc/...' but did not in '.'. I'm not sure why it's not showing as a file rename however!
config/processCcd.py
Outdated
| """ | ||
| import os.path | ||
|
|
||
|
|
||
| for sub in ("isr", "charImage", "calibrate"): | ||
| path = os.path.join(os.path.dirname(__file__), sub + ".py") | ||
| if os.path.exists(path): | ||
| getattr(config, sub).load(path) | ||
| getattr(config, sub).load(path) |
| config.doMakeSourceTable = True | ||
| config.doSaveWideSourceTable = True | ||
| config.transformSourceTable.load(os.path.join(os.path.dirname(__file__), "transformSourceTable.py")) | ||
| config.ignoreCcdList = [9, 104, 105, 106, 107, 108, 109, 110, 111] |
There was a problem hiding this comment.
Again, not for this ticket, but it seems that, except for the load, these are the same settings as in processCcdDriver.py and, if they are meant to be the same, it would probably be better if they could be inherited from there to avoid things getting out of sync (@sr525?).
config/skyAnalysis.py
Outdated
| @@ -2,5 +2,4 @@ | |||
|
|
|||
|
|
|||
| config.colorterms.load(os.path.join(os.path.dirname(__file__), "colorterms.py")) | |||
| config.refObjLoader.load(os.path.join(os.path.dirname(__file__), "..", "filterMap.py")) | |||
There was a problem hiding this comment.
Oh, actually, as of lsst-dm/pipe_analysis@ebf9570, this can be deleted (sorry, I should've done it on that ticket, so feel free to punt!).
There was a problem hiding this comment.
This line now reads:
config.refObjLoader.load(os.path.join(os.path.dirname(__file__), "filterMap.py"))
i.e., with the ".." removed. Am I still okay to remove here following your comment?
There was a problem hiding this comment.
Yeah, the task itself is now gone, so this is a config file to nowhere...
There was a problem hiding this comment.
Removed. This file now only loads colorterms.py.
leeskelvin
force-pushed
the
tickets/DM-27255
branch
6 times, most recently
from
e3f6c03
to
886ad83
Compare
SuprimeCam no longer supported. Remove config/hsc.
leeskelvin
force-pushed
the
tickets/DM-27255
branch
from
886ad83
to
f1f579d
Compare
SuprimeCam no longer supported. Remove config/hsc.