-
Notifications
You must be signed in to change notification settings - Fork 0
save entitysdk entities #460
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
base: main
Are you sure you want to change the base?
Conversation
|
pinging @darshanmandge on all my unmerged PRs |
12c33f5 to
f040404
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.
The code looks fine to me!
Just wondering, is there an example notebook so I can actually run this code? If not it would be great to add a minimal working example.
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 is a notebook examples/A-.../ion_channel_modeling.ipynb @chr-pok
Currently failing when the useIon model is built within the task
| grid_scan.execute() | ||
|
|
||
| with pytest.raises(ValueError, match=re.escape("Unknown properties: [np.str_('INVALID')]")): | ||
| with pytest.raises(ValueError, match=r"Unknown properties:.*INVALID"): |
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 changed this is in main recently to:
[np.str_('INVALID')]
Do we know why this change was needed? It looks like connectome utilities hasn't been changed @chr-pok
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.
No idea! I cannot reproduce locally. I suppose some dependencies that are used by connectome utilities have changed perhaps.
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.
Ok, then this is a good fix for now I think!
This has to be merged once this entitysdk PR has been merged: openbraininstitute/entitysdk#139