Skip to content
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

Fix slm requirement in Python API #15

Merged
merged 4 commits into from
Jun 28, 2024
Merged

Conversation

CodyCBakerPhD
Copy link
Member

The spec specifies the SLM of a stimulus site is not required: https://github.com/catalystneuro/ndx-patterned-ogen/blob/main/spec/ndx-patterned-ogen.extensions.yaml#L124

But I was unable to use the API without one specified

This should fix the issue

@@ -1,4 +1,5 @@
groups:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added spacing between type definitions here in the spec to make them easier to visually identify

@CodyCBakerPhD
Copy link
Member Author

CodyCBakerPhD commented Jun 26, 2024

@rly Couple of things here

i) I get a build warning on file write

C:\Users\theac\anaconda3\envs\leifer_lab_to_nwb_created_5_13_2024\Lib\site-packages\hdmf\build\objectmapper.py:671: MissingRequiredBuildWarning: PatternedOptogeneticStimulusSite 'PatternedOptogeneticStimulusSite' is missing required value for attribute 'spatial_light_modulator'.
  warnings.warn(msg, MissingRequiredBuildWarning)

But file roundtrips just fine; also confused why it thinks it's required in the object mapper

Not too worried since the file reads fine, just curious / letting you know

ii) the testing suite for ndx-extension template seems to be failing due to numpy 2.0 issues; advice on how to resolve generally (besides hard pinning upper bound on all extensions?)

@rly
Copy link

rly commented Jun 26, 2024

@rly Couple of things here

i) I get a build warning on file write

C:\Users\theac\anaconda3\envs\leifer_lab_to_nwb_created_5_13_2024\Lib\site-packages\hdmf\build\objectmapper.py:671: MissingRequiredBuildWarning: PatternedOptogeneticStimulusSite 'PatternedOptogeneticStimulusSite' is missing required value for attribute 'spatial_light_modulator'.
  warnings.warn(msg, MissingRequiredBuildWarning)

required: false is not valid on links. I'll raise an issue to catch that. It's a common mistake. Use quantity: "?" instead. Strange that it reads fine; the default is quantity: 1 meaning it's required.

I'll look into the template.

@CodyCBakerPhD
Copy link
Member Author

Though I guess, oddly, the PR on catalystneuro/ndx-microscopy#14 doesn't have that issue with the tests so maybe it's just this extension

@CodyCBakerPhD
Copy link
Member Author

Yeah OK, pinning here fixed it but not sure about other extensions. Just wanted you to be aware

@CodyCBakerPhD CodyCBakerPhD requested a review from rly June 26, 2024 20:49
@rly
Copy link

rly commented Jun 26, 2024

Yeah OK, pinning here fixed it but not sure about other extensions. Just wanted you to be aware

Thanks. It turns out that the "upgraded" workflow was not actually upgrading the packages. That one should have passed. The others should fail because older versions of pynwb and hdmf, which are pinned for testing, did not set an upper limit on numpy, so numpy 2 ends up being installed. I will update the pinned versions in the template.

@CodyCBakerPhD
Copy link
Member Author

How does this look @rly ?

@CodyCBakerPhD CodyCBakerPhD merged commit f96acab into main Jun 28, 2024
34 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the fix_slm_requirement branch June 28, 2024 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants