search

AllenSWDB / allenswdb.github.io

Other
3 stars 6 forks source link

Updating stimulus_table #41

Closed yavorska-iryna closed 1 month ago

matchings commented 2 months ago

@yavorska-iryna Resetting the index only changes the stimulus_presentations_id by one value. If you reindex, it starts at 0, if you don't, it starts at 1. I don't see how that would change how informative or meaningful it is, so my thought was to reduce number of lines of code for simplicity.

If anything, it makes it less meaningful because it is changing the original data in the table, which is generally not recommended.

If someone really wants the index to start at 0, they can reindex it themselves, but i don't think we should be including extra lines of code for an operation that changes the data that is provided by the SDK / NWB files.

matchings commented 2 months ago

@JEElsner This issue (#41) is only part of the fix related to #48. This PR is about changing the code such that it will work with version 1.1.0 of the Visual Behavior Ophys dataset. The goal of #48 is to make sure that the NWB files associated with v1.1.0 are registered as a data asset in code ocean. The full fix is to put both of those things together so that the code and the files line up. But thanks for identifying the relationship here :)

matchings commented 1 month ago

@JEElsner I believe the essential changes have been made for this PR. Would you be able to test the changes in the built databook?

Also, I realized that the fixes for this PR didn't entirely solve #86 because of the way the plotting functions in that notebook worked, so I implemented a slightly different solution here. Either one would work though.

JEElsner commented 1 month ago

Test build started! I'll update you tomorrow with the results.

matchings commented 1 month ago

@JEElsner I think this can be merged if the errors are resolved on your end (thanks for fixing the last few typos)

We should open a separate ticket to address how to deal with the warnings when loading the cache. Thats likely a change to the SDK.

JEElsner commented 1 month ago

@matchings Gotcha. There's still a KeyError in VBO-ExperimentData, but I think that's it.