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-36071: Deprecate kernelSize* parameters in PSF determiner tasks #296

Merged
merged 11 commits into from Oct 14, 2022

Conversation

arunkannawadi
Copy link
Member

No description provided.

@arunkannawadi arunkannawadi force-pushed the tickets/DM-36071 branch 4 times, most recently from 6e2227d to a3e7aed Compare October 11, 2022 13:45
@arunkannawadi arunkannawadi marked this pull request as ready for review October 11, 2022 20:51
Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

Looks good, though I suspect it might be better to squash some commits, just given the number of commits to the number of actual lines changed overall.

If you don't squash, you've got one commit message with a typo, in which the actual word "typo" should be "type".

)

def validate(self):
# TODO: DM-36311: This entire method may be removed after v25.
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to be specific about which lines are to be removed, on the off chance something else gets added to this method before that ticket happens.

@@ -34,22 +34,51 @@ class BasePsfDeterminerConfig(pexConfig.Config):

This is fairly sparse; more fields can be moved here once it is clear they are universal.
"""
stampSize = pexConfig.Field(
Copy link
Member

Choose a reason for hiding this comment

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

I see you used the new type annotation syntax elsewhere on this PR, but not here. Any reason why? (I don't have a preference, except for consistency.)

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 did that in one file, where there were only two config fields. I'm happy to change it in the files I'm touching.

@arunkannawadi
Copy link
Member Author

Squashed some commits (reduced 19 to 10) and added one more.

@arunkannawadi arunkannawadi merged commit 169e04b into main Oct 14, 2022
@arunkannawadi arunkannawadi deleted the tickets/DM-36071 branch October 14, 2022 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants