catalystneuro / ndx-patterned-ogen

patterned (holographic) optogenetic extension to NWB standard
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Change quantitative properties to add units to name #6

Closed alessandratrapani closed 7 months ago

alessandratrapani commented 7 months ago

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

CodyCBakerPhD commented 7 months ago

Some conflicts here now

CodyCBakerPhD commented 7 months ago

The main reasoning for this was to be able to set a unit attribute then on each quantitative dataset, right?

Would you be able to share an example file using this alteration to see how it looks in practice?

alessandratrapani commented 7 months ago

The main reasoning for this was to be able to set a unit attribute then on each quantitative dataset, right?

Yes, sorry it's still work in progress. I will have a meeting with Ryan tomorrow to solve this

alessandratrapani commented 7 months ago

Here is an example file. Not quite sure why tests are failing here, but not locally.

CodyCBakerPhD commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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

alessandratrapani commented 7 months ago

@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 commented 7 months ago

I think this looks great this way