Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH 771: Remove redundant 'sham_change' column from trials #1397

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

kschelonka
Copy link
Contributor

@kschelonka kschelonka commented Feb 27, 2020

Response to #771. I checked for the use of "sham_change" in the repo -- I found some uses internally in the function that produces the trial dataframe, but no downstream functions in the AllenSDK.

Functions using the column:
all in allensdk.brain_observatory.behavior.trials_processing (outside of tests)

trial_data_from_log (used only by get_trials, could be made private)
get_change_time (function defined in local scope of get_trials)
get_change_time_frame_response_latency (used only by get_trials_v0, which seems deprecated/unused? Could be made private/removed.)

@kschelonka kschelonka self-assigned this Feb 27, 2020
@wbwakeman wbwakeman added the braintv relates to Insitute BrainTV program label Mar 2, 2020
@njmei
Copy link
Contributor

njmei commented Mar 5, 2020

I think it would be good to note in #771 which exact function(s) use(s) the "sham_change" column.

Copy link
Contributor

@njmei njmei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, looks good!

@kschelonka
Copy link
Contributor Author

I think it would be good to note in #771 which exact function(s) use(s) the "sham_change" column.

Ok, I can do that. It's only functions that go into producing the actual trials output.

@kschelonka kschelonka merged commit f4dd74b into rc/1.6.0 Mar 5, 2020
@kschelonka kschelonka deleted the GH-771/update/remove-redundant-trial-col branch March 5, 2020 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
braintv relates to Insitute BrainTV program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants