AllenNeuralDynamics / aind-dynamic-foraging-data-utils

Tools for the analysis of behavior and neural data from the dynamic foraging task
MIT License
2 stars 0 forks source link

Function for aligning continuous variables to trial by trial events #8

Open alexpiet opened 8 months ago

alexpiet commented 8 months ago
alexpiet commented 8 months ago

Common time stamps rate should be given as an input. Default value should be the continuous variable rate.

rachelstephlee commented 8 months ago

Use mindscope_utilties, event_triggered_response, use the commit af2b70a

rachelstephlee commented 8 months ago

I have a first pass running through this, but there are some data that have missing activity during the event_times. should we ignore those events ?

alexpiet commented 8 months ago

If we have missing data around an event, I think we should return the aligned data for that event as NaN where it is missing. The NaN could be the entire, or part of, the window for a specific event.

rachelstephlee commented 8 months ago

Coverage only passes the unit test if all functions are in a class. Would you want that or should we skip on the src files?

Not sure if it'll fit with how you want this to be lightweight.

jsiegle commented 8 months ago

Since this will be such a widely used utility function, I would propose putting this in a different repo that is shared across projects. I think the MindScope utilities version is a good starting point, but we should move it to a package we maintain. See the latest version of the planning document for my proposed division of functionality.

rachelstephlee commented 8 months ago

We should meet once Alex is back then-- he envisioned something more lightweight than multiple packages, each specified for a type of data or for behavior.

jsiegle commented 8 months ago

Happy to chat about it! It may make sense to implement this initially in dynamic-foraging-analysis, but eventually it will need to be reused across many projects

alexpiet commented 7 months ago

Coverage only passes the unit test if all functions are in a class. Would you want that or should we skip on the src files?

Not sure if it'll fit with how you want this to be lightweight.

Seems unnecessary to have all functions in a class. I'm not familiar with the tests, so can we disable that requirement?

alexpiet commented 7 months ago

Happy to chat about it! It may make sense to implement this initially in dynamic-foraging-analysis, but eventually it will need to be reused across many projects

My concern is creating a repository that has so many people using it that it becomes impossible to update (AllenSDK). This function is obviously something that other projects will use, but we should be very intentional about what goes in a shared repo.

jsiegle commented 7 months ago

My inclination to divide things across multiple repos also stems from my experience with the AllenSDK. If all analysis code lives in one package, it starts to develop idiosyncrasies that make it difficult for others to reuse/contribute to. The biggest problem with the AllenSDK was probably the fact that numerous pinned dependencies made it impossible to install in an existing environment, but a tangential problem was that its monolithic nature meant that very few people inside the Institute actually used it as a starting point for exploratory analysis. Obviously we are not trying to create something on the same scale, but I think it's important to plan for the type of code ecosystem we want to end up with before investing significant time building shared packages.

Looking forward to chatting more about this!

alexpiet commented 2 months ago