-
Notifications
You must be signed in to change notification settings - Fork 24
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
Getter Docstrings #1185
Getter Docstrings #1185
Conversation
for more information, see https://pre-commit.ci
After the meeting today, I think this kind of task would be a really good benchmark for AI agents / LLMs / prompts. Some quantifiable questions:
With that in mind, it might be worthwhile to branch off from neuroconv with a dedicated ai_docstrings_benchmark branch before merging these changes. |
|
||
Returns | ||
------- | ||
type |
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.
Probably not useful, what do you think?
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 one is actually fairly reasonable. The docstring doesn't provide a ton of info, but it stays consistent with the standard of the repo.
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 meant the type
no the whole docstring.
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.
It's accurate right? type(class) is type.
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.
Accurate I agree but I don't think it adds too much. There is an Any somewhere there as well.
Just commenting, don't really know how I feel, I don't think it subtracts.
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.
Looks like an improvement to me, some small comments but should be good to go after that.
@@ -99,7 +113,14 @@ def get_metadata_schema(self) -> dict: | |||
return metadata_schema | |||
|
|||
def get_metadata(self) -> DeepDict: | |||
"""Auto-fill as much of the metadata as possible. Must comply with metadata schema.""" | |||
""" | |||
Auto-fill as much of the metadata as possible. Must comply with metadata schema. |
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.
Probably not useful this one.
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.
Seems fine to me.
src/neuroconv/datainterfaces/ophys/scanimage/scanimageimaginginterfaces.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Heberto Mayorquin <[email protected]>
Co-authored-by: Heberto Mayorquin <[email protected]>
…nterfaces.py Co-authored-by: Heberto Mayorquin <[email protected]>
Co-authored-by: Heberto Mayorquin <[email protected]>
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.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1185 +/- ##
==========================================
- Coverage 90.69% 89.65% -1.05%
==========================================
Files 129 129
Lines 8189 8378 +189
==========================================
+ Hits 7427 7511 +84
- Misses 762 867 +105
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Added Returns section to all getter docstrings.
Fixes #1097
Cline (Claude 3.5 Sonnet) helped. It was so close to just getting it right after the first couple attempts, but then took a lot of coaching to get those last few...