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.

Sign up for GitHub

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

Jump to bottom

Ticket/psb-171: Various fixes for VBO release #2707

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Ticket/psb-171: Various fixes for VBO release #2707

merged 1 commit into from
Aug 11, 2023

Conversation

morriscb
Copy link
Contributor

@morriscb morriscb commented Jul 25, 2023 •
edited
Loading

  • Add additional doc to experiment level cell specimen return.
  • Change to Int64 to have N/A values in the metadata table instead of -99 as requested. Modify enforce_df_int_typing to return Int64 types.
  • Change stimulus table name from repeat to movie_repeat
  • Modify location of spurious omitted stimulus check and move code to static_method function. Fixes incorrect detection of 5 minute grey screen.
  • Update unittests and test data.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@morriscb morriscb changed the base branch from master to rc/2.16.0 July 28, 2023 22:08
@morriscb morriscb force-pushed the ticket/PSB-171/dev branch from d4ef208 to d81262c Compare August 1, 2023 02:17
@morriscb morriscb requested a review from mikejhuang August 1, 2023 16:59
@morriscb morriscb marked this pull request as ready for review August 1, 2023 16:59
@morriscb morriscb changed the base branch from rc/2.16.0 to rc/2.15.2 August 8, 2023 20:47
@morriscb morriscb changed the base branch from rc/2.15.2 to rc/2.16.0 August 8, 2023 20:47
@morriscb morriscb force-pushed the ticket/PSB-171/dev branch from d36597a to 264764e Compare August 8, 2023 20:48
@morriscb morriscb requested review from mikejhuang and removed request for mikejhuang August 9, 2023 01:20
mikejhuang
mikejhuang reviewed Aug 9, 2023
View reviewed changes
Copy link
Contributor

@mikejhuang mikejhuang left a comment

Choose a reason for hiding this comment

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

I understand that the output from these fixes has been approved by Marina. However, I noticed one of your action items wasn't implemented. "Drop max_correction values from session cell table."
https://alleninstitute-brainscience.atlassian.net/browse/PSB-171?focusedCommentId=84596

allensdk/brain_observatory/behavior/data_objects/stimuli/presentations.py
found, return input_df unmodified.
"""
if "omitted" in input_df.columns and len(input_df) > 0:
first_row = input_df.iloc[0]
if not pd.isna(first_row["omitted"]):
Copy link
Contributor

@mikejhuang mikejhuang Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any value for first_row{"omitted"] such that is not pd.isna(first_row["omitted"]) and if first_row["omitted"] is False?

Edit: 0 and False would qualify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Omitted can be nan for certain sessions. This is basically just a cautions test as the behaviors can be different for numpy and pandas NA/nan values (pd.NA returns errors as ambiguous and if np.nan evaluates as true). If the omitted value is nan we can continue as it's not "True".

allensdk/brain_observatory/behavior/data_objects/stimuli/presentations.py Outdated
if not pd.isna(first_row["omitted"]):
if first_row["omitted"]:
df = df.drop(first_row.name, axis=0)
return df
input_df.drop(first_row.name, axis=0, inplace=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a reason for changing inplace=True for this. This method is returning the df.

Copy link
Contributor

Choose a reason for hiding this comment

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

Setting inplace=True might cause unexpected issues if this method is implemented elsewhere with the same dataframe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We usually just use the functions to overwrite the dataframe in the code but I've changed it back to df = df[modify.]

@morriscb
Copy link
Contributor Author

Dropping the "max_correction" values ended up breaking unittests in a non-trivial way so I ended up not doing it after approval from Marina as dropping the columns is not strictly necessary or fixing any bug.

@morriscb morriscb requested a review from mikejhuang August 10, 2023 22:49
mikejhuang
mikejhuang approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@mikejhuang mikejhuang left a comment

Choose a reason for hiding this comment

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

thanks for the changes

@morriscb morriscb force-pushed the ticket/PSB-171/dev branch from 5265529 to 5a6fbbd Compare August 11, 2023 16:33
@morriscb
Updates before VBO re-release
64e334f 
Create function for dropping first row omitted.
Add to "from_stimulus_file"
Change repeat -> movie_repeat
Add description of max_correction usage to docstring.
Add Int64 check to enforce_int_typing.
Change name of movie accessor.
@morriscb morriscb force-pushed the ticket/PSB-171/dev branch from 5a6fbbd to 64e334f Compare August 11, 2023 16:34
@morriscb morriscb merged commit 7af2406 into rc/2.16.0 Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikejhuang mikejhuang mikejhuang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@morriscb @mikejhuang