search

AllenInstitute / AllenSDK

code for reading and processing Allen Institute for Brain Science data
https://allensdk.readthedocs.io/en/latest/
Other
340 stars 150 forks source link

Bugfix/nightly/test/220915 #2537

Closed danielsf closed 1 year ago

danielsf commented 1 year ago

This PR finalizes the move from experiment_container_id to ophys_container_id.

The first commit is needed to fix a nightly test that was breaking on GitHub Actions.

The second commit resulted when I noticed a TODO: that seemed redundant with work that was already accomplished in #2501. Feel free to argue that that second commit should be reverted, if I got over-eager. I think everything got changed self-consistently, though.

I did have to mark a unit test with skip until such time as we update the published VBO data release to reflect the new data model.

danielsf commented 1 year ago

The test that I marked skip is meant to catch is whenever we do what this PR does: change the code in such a way that you can no longer load an NWB file from the published cache. There is a cache somewhere on Isilon that corresponds to the 1.0.1 release of the data and the test just tries to read a BehaviorSession and an BehaviorOphysExperiment from that cache.

Maybe that test is too paranoid and should just be deleted. I'm not sure there's a simple way to implement the test such that it always works with the most up-to-date release candidate. We would have to construct a new data release, store it in a tmp dir, and then try to read from it.

Really, I guess what we want is a to_nwb -> from_nwb round trip test.

This test already checks that for a BehaviorOphysExperiment

https://github.com/AllenInstitute/AllenSDK/blob/master/allensdk/test/brain_observatory/behavior/test_behavior_ophys_experiment.py#L27-L47

Should I just add a similar test for BehaviorSession and then delete the skipped test?

aamster commented 1 year ago

I'm fine with deleting in favor of a roundtrip test that you proposed.