AllenInstitute / visual_behavior_analysis

Python package for analyzing behavioral data for Brain Observatory: Visual Behavior
Other
21 stars 6 forks source link

removing sdk_translator #721

Closed alexpiet closed 3 years ago

alexpiet commented 3 years ago

@matchings @dougollerenshaw @DowntonCrabby @yavorska-iryna @farznaj

To my knowledge, the only code that was using the translator/allensdk_translator/ functions were the behavior model, which I have just updated to use the loading/data_access functions. Rather than maintaining these old functions, I propose removing them. Any objections?

dougollerenshaw commented 3 years ago

@alexpiet there are tests associated with the module that are now failing. See tests/translator/allensdk_sessions/test_allendk_session_translation.py

dougollerenshaw commented 3 years ago

I recently added instructions for running tests locally to the project README. That'll let you run the same tests on your local machine that CircleCI will run on the cloud. That way you can make sure everything passes without having to commit and wait for the CircleCI tests.

alexpiet commented 3 years ago

@alexpiet there are tests associated with the module that are now failing. See tests/translator/allensdk_sessions/test_allendk_session_translation.py

I dont understand how a test can fail if we are removing the functions?

dougollerenshaw commented 3 years ago

There is a test on the module that fails because it can't import the module: https://github.com/AllenInstitute/visual_behavior_analysis/blob/814a222aa51fb421375fbb110cb8c04893b98954/tests/translator/allensdk_sessions/test_allendk_session_translation.py#L5

It seems to me that this entire folder should be removed as part of this PR: https://github.com/AllenInstitute/visual_behavior_analysis/tree/remove_sdk_translator/tests/translator/allensdk_sessions

alexpiet commented 3 years ago

ugh, I thought I did remove the entire folder. I do not understand the circleci tests at all.

edit: I missed that the tests were in a different folder. I just committed a change to remove the associated tests as well.

dougollerenshaw commented 3 years ago

All passing now. Thanks! I'll merge.