-
Notifications
You must be signed in to change notification settings - Fork 6
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
Merge dc2/run2.1 DESC updates into new dc2/run2.2 branch #134
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.
These seem fine to me. Have we already tried these configs with v18.1.0?
@jchiang87 not yet. I will take care of it as soon as I am done with the dr1b metacal rerun request |
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.
are there some DIA stuff in the templates that we should add now that the opportunity is presenting itself?
@@ -30,3 +30,4 @@ | |||
config.doAddDistortionModel = False | |||
config.qa.doThumbnailOss = False | |||
config.qa.doThumbnailFlattened = False | |||
config.doCrosstalkBeforeAssemble = False |
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.
This is reasserting the upstream default, which is usually a no-no. This is not proposed as a merge back to the main DM codebase, nor will it be, right? Just wanted to check in on this line as a word of caution, and to check what the final resting place for this line is intended to be.
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.
(Had this only been to the imsim isr config I wouldn't have said anything, fwiw)
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.
This is only for the dc2/run2.2 branch. Any changes that we intend to merge back to master will likely happen after Gen-3 is more fully ready and will be coordinated with the DM team.
We never worked out getting the config.doCrosstalkBeforeAssemble setting to stick, unless we set it in the upper level config/isr.py. I am hoping this will become a non-issue with upcoming versions of DM.
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.
OK, not a problem then, though that is interesting/worrying about not being able to get it to stick 🤔
I imagine that when any significant merge-back happens I'll be paying attention then, so will file under "ignore for now" 🙂
@johannct Would you prefer that I hold off on a merge until you test? I suspect it might be easier to test, if the changes are available in a dc2/run2.2 branch? As for DIA template updates, I'm keeping an eye on #122 The hope is to see that merged into obs_lsst master, and then we use that as our test case to work out backporting a simple update back into the 18.1.0 release. So, I'm not pulling that into this branch right now. |
@heather999 you can go ahead with the merge. We will commit to it if needs be |
For DC2 Run2.2i, DESC is moving to use DM 18.1.0. Merging in updates from our dc2/run2.1 branch into the new dc2/run2.2 branch which is based on 18.1.0