Skip to content

Conversation

@parejkoj
Copy link
Contributor

@parejkoj parejkoj commented Oct 23, 2024

Placeholder, including the commits from DM-47084 (will rebase onto main once that merges) - rebased to main, only the one final RFC commit.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

Passing various inputs as keyword-only arguments is certainly much cleaner. Aside from the minor inline reviews, two larger changes requests are:

  1. Since GetDcrTemplateTask inherits from GetTemplateTask, the API changes to getOverlappingExposures should be reflected in the former as well.
  2. Although DM-CCB has approved the change, a user of this public method might get annoyed by the changing API without a deprecation warning. I'd suggest renaming the method to getOverlappingCoaddExposures or something, and define getOverlappingExposures (with deprecated decorator) as something that rips out the quantities from inputs and calls getOverlappingCoaddExposures.

@@ -123,15 +123,24 @@ def __init__(self, *args, **kwargs):

def runQuantum(self, butlerQC, inputRefs, outputRefs):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def runQuantum(self, butlerQC, inputRefs, outputRefs):
def runQuantum(self, butlerQC, inputRefs, outputRefs):
# Docstring inherited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we really need to put this everywhere there isn't a docstring? I don't see this mentioned in the dev guide, and always figured that if there isn't a docstring it was implied.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it was more of a suggestion here. The lack of docstring jumps out sometimes, and seeing that comment reassures that it's not needed. This could be just me.

@parejkoj
Copy link
Contributor Author

The problem with modifying GetDcrTemplateTask is that it's even less well-tested than the base class (which doesn't have any unittests, only ci_ap). I'll can do my best to update it, but I was leaving it intentionally broken so that if someone tries to use it it will be clear it needs work.

@parejkoj
Copy link
Contributor Author

Similarly, I'll change names and add a deprecated version, but I can't guarantee that it will work because it won't be tested.

@parejkoj
Copy link
Contributor Author

Following up: I'm not going to touch GetDcrTemplateTask: it will need a new runQuantum, too, and it is not used anywhere in our current code, so there's no way to even theoretically test it. I'll file a separate ticket to bring it up to date, and let the DCR folks sort out whether they want to do that, or remove it.

Bring `runQauntum` more in line with how we want arguments to be
handled (load the inputs and pass them as named arguments).
Refactor `getOverlappingExposures` so that it can be used more easily
as outside of `runQuantum`.

Change method name to `getExposures` allow deprecation warning on original
`getOverlappingExposures`.
@parejkoj
Copy link
Contributor Author

I've filed https://rubinobs.atlassian.net/browse/DM-49079 about the Dcr template code. I believe I've incorporated all the rest of your suggestions.

@parejkoj parejkoj merged commit 8f1d76f into main Feb 21, 2025
2 checks passed
@parejkoj parejkoj deleted the tickets/DM-47085 branch February 21, 2025 18:36
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.

2 participants