-
Notifications
You must be signed in to change notification settings - Fork 23
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
Schema inference enhancements #1037
Conversation
Oh, thanks for taking over the fixing. Let me know when is ready and I can run in the current example to see if it becomes clearer. Current example for future reference: /home/heberto/development/neuroconv/src/neuroconv/basedatainterface.py:81: UserWarning: The argument_name 'nwbfile' from the docstring not occur in the method signature, possibly due to a typo.
return get_json_schema_from_method_signature(self.add_to_nwbfile, exclude=["nwbfile", "metadata"])
/home/heberto/development/neuroconv/src/neuroconv/basedatainterface.py:81: UserWarning: The argument_name 'metadata' from the docstring not occur in the method signature, possibly due to a typo.
return get_json_schema_from_method_signature(self.add_to_nwbfile, exclude=["nwbfile", "metadata"])
/home/heberto/development/neuroconv/src/neuroconv/basedatainterface.py:81: UserWarning: The argument_name 'The default is False (append mode).' from the docstring not occur in the method signature, possibly due to a typo.
return get_json_schema_from_method_signature(self.add_to_nwbfile, exclude=["nwbfile", "metadata"]) |
@h-mayorquin I am unable to reproduce the Can you give this a try and see how it works? |
@@ -315,7 +315,6 @@ def add_to_nwbfile( | |||
|
|||
metadata['Ecephys']['ElectricalSeries'] = dict(name=my_name, description=my_description) | |||
|
|||
The default is False (append mode). |
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.
@CodyCBakerPhD it seems that it came from here. Let me test, anyway, this was an error.
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.
You would be correct!
https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/datainterfaces/ecephys/baserecordingextractorinterface.py#L318 would be interpreted as an argument not the description of the argument it is attached to
After my last changes and yours now I don't get any of those warnings but I saw the tests are failing. Let's see how they do on this run. |
@h-mayorquin Tests passing now Might be worth cutting a hotfix release for this |
Yeah, why not, plus I know how to fix images so they display correctly in pipy and I forgot to do it in the latest release. |
@@ -102,6 +102,9 @@ def get_json_schema_from_method_signature(method: Callable, exclude: Optional[li | |||
exclude = exclude or [] | |||
exclude += ["self", "cls"] | |||
|
|||
split_qualname = method.__qualname__.split(".")[-2:] | |||
method_display = ".".join(split_qualname) if "<" not in split_qualname[0] else method.__name__ |
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.
What is the <
case for?
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.
Sometimes <locals>
(local namespaces domain) shows up for basic functions. Think it depends on how those functions are defined/imported, but it happens in the tests and I think would happen if you locally defined a data interface in the same script you are trying to run this with
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1037 +/- ##
=======================================
Coverage 90.41% 90.42%
=======================================
Files 129 129
Lines 7919 7924 +5
=======================================
+ Hits 7160 7165 +5
Misses 759 759
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@h-mayorquin Thanks for reporting the bug
Correcting a typo, enhancing warning info, and debugging exclusion interaction here