-
Notifications
You must be signed in to change notification settings - Fork 12
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-44305: Implement Gaussian Process interpolation over bad pixels #376
Conversation
e451ab4
to
27b7a91
Compare
d706b5e
to
ed31aee
Compare
2e7c572
to
a5c9951
Compare
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 mostly looks good. I have a lot of comments, but they are mostly related to bringing this into conformity with the LSST stack coding standards, rather than algorithmic changes or anything like that.
The one major thing is that there need to be unit tests for InterpolateOverDefectGaussianProcess
and the other new classes. The standalone functions don't necessarily need their own tests as long as they are run through the unit tests for the classes.
Here are a few other general points, which I included in the comments, but didn't make a note of every time I saw them:
- For some reason the github Actions are not complaining about this, but a lot of the docstring lines are too long. The rule from the Dev Guide is 79 characters: https://developer.lsst.io/python/style.html#docstring-and-comment-line-length-must-be-less-than-or-equal-to-79-columns. For lines of code, the maximum is 110 characters.
- In the docstrings, the variable type needs to be surrounded by backquotes, i.g.
`int`
. - Also in the docstrings, the summary sentence should be in the imperative sense, and should be formatted with the opening triple quotes on the same line as the sentence.
- We try to be as consistent as possible with capitalization. You have almost everything in snake_case, but a few variables are camelCase. It would be best to change those to snake_case too.
- If possible, pass the logger into the class and use that instead if
warnings
. If not, I think it would be better to use thelogging
package rather thanwarnings
, just because it is what is most used in the stack.
4719d5a
to
e427bfb
Compare
e427bfb
to
7d133e0
Compare
See this ticket --> https://rubinobs.atlassian.net/browse/DM-44305