-
Notifications
You must be signed in to change notification settings - Fork 0
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-44548 Patches for calibration rehearsal 1 (CR1) #49
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.
I'm not sure what the doDropStats
is about; it doesn't seem to be used or tested that I can see.
doDropStats = pexConfig.Field( | ||
dtype=bool, | ||
doc="Drop deprecated stats files?", | ||
default=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.
If these are deprecated, why is the default not True? Also, is there a ticket to remove any deprecated files (if so it should be tagged here with a TODO; if not it should be filed and then tagged here with a TODO.)
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.
They're not formally deprecated, and this was a quick-fix to avoid the problem.
The problem: When I updated cp_verify
to get the analysis_tools
development unblocked, I was more explicit about which tasks have filter dimensions and which don't. This changed the flatStats
dimensions to include physical_filter
, matching the flatResults
dataset, but causing conflicts with the existing definition in /repo/embargo
. Since the flatStats
never gest used outside of cp_verify
, but the flatResults
do (they're the input to analysis_tools
), having this dataset get dropped avoided the butler problem.
The correct path forward is to RFC the *Stats
datasets to be deprecated and removed, and I don't think that will have any objections, but having this shortcut is helpful until I have the time for the proper RFC deprecation.
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.
Okay, then I would suggest updating the doc string here. Also, I'm not sure where this is even set to True
!
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 think the pipeline that had this set to true was rebased out of existence due to RFC-1013. I'll clarify the doc string.
Patches that were needed for the calibration rehearsal, that we now want to include on main:
outputStats
connection to avoid butler dimension mismatches on this to-be-deprecated dataset.