-
Notifications
You must be signed in to change notification settings - Fork 150
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
Ticket/2450/supplemental/columns #2451
Conversation
supplemental_df = pd.DataFrame( | ||
data=self.args['supplemental_columns']) | ||
|
||
columns_to_patch = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just use pd.merge
here instead of patch_df_from_other
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we did just use pd.merge
, I'd want to wrap it in a function that we could test to make sure that the columns we are adding get added the way we expect. patch_df_from_other
is already tested. I'd rather keep this as it is.
@@ -47,6 +47,18 @@ class VBN2022MetadataWriterInputSchema(argschema.ArgSchema): | |||
"{ecephys_nwb_dir}/{ecephys_nwb_prefix}_{ecephys_session_id}.nwb") | |||
) | |||
|
|||
supplemental_columns = argschema.fields.List( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this argument would be better named as supplemental_data
. supplemental_columns
makes it seem like it is a list of column names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -47,6 +47,18 @@ class VBN2022MetadataWriterInputSchema(argschema.ArgSchema): | |||
"{ecephys_nwb_dir}/{ecephys_nwb_prefix}_{ecephys_session_id}.nwb") | |||
) | |||
|
|||
supplemental_columns = argschema.fields.List( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this rather be an input file? Passing a long list of dicts through the command line would be cumbersome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to proliferate the number of input files we have to keep track of. These input.jsons are large, but they have the virtue of carrying everything we need in one package.
I also don't see users specifying this field (or, or that matter, probes_to_skip
) on the command line.
I'd rather leave this as it is.
As part of the 2022 VBN release, we are adding some hand annotations not currently represented in the LIMS database to the ecephys_sessions table. Rather than update the schema of the LIMS database (a change that would have implications for all previously-collected ecephys data), we are adding functionality to the VBN metadata_writer that allows us to add columns to the ecephys_sessions table by hand. This PR encompasses that functionality. The new
supplemental_columns
entry in the metadata_writer schema should look something like this