-
Notifications
You must be signed in to change notification settings - Fork 94
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
Improve code quality and checking #310
Conversation
@@ -122,14 +149,24 @@ def _get_relative_dom_efficiency(self, frame, om_key): | |||
|
|||
|
|||
class I3FeatureExtractorIceCubeDeepCore(I3FeatureExtractorIceCube86): | |||
"""...""" | |||
"""Class for extracting reconstructed features for IceCube-DeepCore.""" |
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 an IceCuber will recognize what is meant with "reconstructed features" here. I think "Class for extracting unfolded waveform pulses and associated OM information, such as position, orientation, etc." is more descriptive.
I don't think you necessarily have to change all the doc strings accordingly in this PR. Perhaps it's best if we discuss language first and iterate on doc string wording in a separate PR in the future.
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 this is a good point, and this PR might be a good time to review our language as it will update all docstrings in the codebase. I'll try to keep this in mind going forward!
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.
@asogaard really nice! I've added a single comment on the feature extractors; but as I mention, I think we can save that for later, if you prefer.
On another note; how does the automatic checks on doc strings and type hints affect our future PR's? Will the checks update the doc strings and type hints like black formatting does? Or will we have to consult output logs?
... It's done... 😮💨 |
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.
Well done!!
As with all PRs of this size, there is a small change of a bug sneaking in, but as the vast majority of changes are purely to the "frosting", I figure it is still okay to merge. Please see the original, updated comments for notes on the major changes. Please also see the slightly updated contributing guidelines. Finally, people who have been holding off on using pre-commit hooks ought to set it up now. I will also talk about this at next week's meeting. |
_make_df
to_convert_to_dataframe
inParquetToSQLiteConverter
, as the former was renamed but the corresponding call was not update to reflect this.Code quality
GitHub actions for checking each of the pre-commit hooks, replacingblack
which was the only thing we checked for before.Build
, for clarity.configupdater
as setup dependency, as it is used ingraphnet.pisa
.After this PR the recommended way to document functions is like:
That is, types and default values are document in the code itself. Any explanation of the logic is done in the docstring. For trivial function, the
Args
andReturns
section on the docstring can be omitted. This should reduce duplication of e.g. type information, and the typing hinting allows for static type checking usingmypy
.Closes #109
Closes #225
Closes #227
Closes #229