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

Change quantitative properties to add units to name #6

Merged
merged 53 commits into from
Feb 5, 2024

Conversation

alessandratrapani
Copy link
Collaborator

@alessandratrapani alessandratrapani commented Jan 24, 2024

Change quantitative properties from attributes to datasets and add unit property as an attribute of each dataset that is numerical.
Additional adjustments:

alessandratrapani and others added 30 commits January 11, 2024 18:21
@CodyCBakerPhD
Copy link
Member

Not quite sure why tests are failing here, but not locally.

Did you try a fresh environment that is set up in exactly the same way the CI does? We as developers can often do little things differently on our own systems

@CodyCBakerPhD
Copy link
Member

It's weird; the shared file has an excitation_lambda field as a part of its PatternedOptogeneticStimulusSite but I don't see that field specified in the schema? Any ideas (is it purely inherited)? I just notice because it doesn't have a unit like the others do

@CodyCBakerPhD
Copy link
Member

@alessandratrapani Thanks a bunch for working on this, it's great to see an example in practice

Just something to leave as a note here; adding lots of tiny datasets to the file may affect standard performance of read operations, especially from the cloud. Which might diminish some of the gains we've made with the latest version of the extension overall

I'm banking on us figuring out better ways to read from the cloud (kerchunk/consolidated metadata) to resolve that, but good to be aware of and consider if the gain in specificity of the metadata values is worth it

@alessandratrapani
Copy link
Collaborator Author

It's weird; the shared file has an excitation_lambda field as a part of its PatternedOptogeneticStimulusSite but I don't see that field specified in the schema? Any ideas (is it purely inherited)? I just notice because it doesn't have a unit like the others do

Yes, it's purely inherited.

@CodyCBakerPhD CodyCBakerPhD changed the title Change quantitative properties from attributes to datasets Change quantitative properties to add units to name Feb 5, 2024
@CodyCBakerPhD
Copy link
Member

@alessandratrapani Is there a stub file with the new attribute name change approach instead of datasets?

@alessandratrapani
Copy link
Collaborator Author

@alessandratrapani Is there a stub file with the new attribute name change approach instead of datasets?

yes, sorry I forgot to update it: here

@CodyCBakerPhD
Copy link
Member

I think this looks great this way

@CodyCBakerPhD CodyCBakerPhD merged commit 1880684 into main Feb 5, 2024
34 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the change_attributes_w_dataset branch February 5, 2024 17:14
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