-
Notifications
You must be signed in to change notification settings - Fork 26
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
Allow search of ids in ElementIdentifiers #187
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #187 +/- ##
==========================================
+ Coverage 70.32% 70.36% +0.04%
==========================================
Files 30 30
Lines 5911 5916 +5
Branches 1381 1382 +1
==========================================
+ Hits 4157 4163 +6
+ Misses 1323 1322 -1
Partials 431 431
Continue to review full report at Codecov.
|
'of the found elements, rather than in the order in which the elements' | ||
'where given for the search. Also the length of the result may be different from the length' | ||
'of the input array. E.g., if our ids are [1,2,3] and we are search for [3,1,5] the ' | ||
'result would be [0,2] and NOT [2,0,None]') |
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 for docval
to do its intended behavior, the function has to be defined as def __eq__(self, **kwargs)
. Given that this function must be def __eq__(self, other)
in order to override the ==
functionality, the docval decorator should be replaced with a docstring. Then it is clear that validation and all the other docval functionality does not run.
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.
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.e., the test in ed95b58 actually tests this and seems fine too
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.
Interesting. So it does work then. Out of curiousity, does the name need to match the name of the variable? Or is it just doing positional arg matching?
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.
When calling ElementIdentifiers == 1
Python will pass as a positional argument, i.e., the error checking in docval happens for this here
Lines 423 to 436 in 8ddae22
parsed = __parse_args( | |
_copy.deepcopy(loc_val), | |
args[1:], | |
kwargs, | |
enforce_type=enforce_type, | |
enforce_shape=enforce_shape, | |
allow_extra=allow_extra) | |
for error_type, ExceptionType in (('type_errors', TypeError), | |
('value_errors', ValueError)): | |
parse_err = parsed.get(error_type) | |
if parse_err: | |
msg = ', '.join(parse_err) | |
raise_from(ExceptionType(msg), None) |
__eq__
function need to match, e.g, if I change the docval argname it something
you would get TypeError: __eq__() got an unexpected keyword argument 'something'
Motivation
This is to allow us to address oruebel/ndx-icephys-meta#9 When creating references to DynamicTable we often don't know the indices of the rows we need but only the list of ids. With this we can now do:
Checklist
flake8
from the source directory.#XXX
notation whereXXX
is the issue number ? By including "Fix #XXX" you allow GitHub to close the corresponding issue.