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

Electrode helper functions #603

Merged
merged 31 commits into from
Mar 11, 2024
Merged

Conversation

CodyCBakerPhD
Copy link
Collaborator

Drafting some electrode helper functions; need to debug them thoroughly and then they need to be wrapped in the API

@CodyCBakerPhD CodyCBakerPhD self-assigned this Feb 7, 2024
@garrettmflynn
Copy link
Member

Updated this PR to show on the frontend. This required adapting some of the code with reference to catalystneuro/neuroconv#447 and commenting out places where the value doesn't exist (e.g. interface.get_metadata()["Ecephys"]["ElectrodeColumns"], as this is something we've defined in memory and sent to the GUIDE for proper constraints on the Electrodes table)

@CodyCBakerPhD
Copy link
Collaborator Author

@garrettmflynn Feel free to accept/merge into #586 if you think this works; I just opened as a separate PR since it was a lot of code (more than a usual suggestion and didn't want to commit directly to that branch since this is rather more experimental)

@garrettmflynn
Copy link
Member

garrettmflynn commented Feb 12, 2024

@CodyCBakerPhD This still isn't fully submitting the Ecephys metadata back to the interface correctly. Can you help debug? I'm using the SpikeGLX-Phy test pipeline.

While I've gotten the metadata to validate on the frontend, this error pops up when submitted, which seems to be related to how the contact_shapes dtype is parsd:

Screenshot 2024-02-12 at 11 40 41 AM

@CodyCBakerPhD
Copy link
Collaborator Author

Can you help debug?

Yes, I'll be on this starting this week

@CodyCBakerPhD
Copy link
Collaborator Author

@garrettmflynn OK, sorry again for taking so long on this; I've added code that unpacks the contact_vector object, which is one of the more exotic numpy-allowed types ('object') - it's essentially just a hacky way of indexing a typical JSON object / table in an array pattern for efficient random access

Let me know if you need any help with follow up issues, or when this is ready for a first big review (looking good so far from what I saw debuggin)

@garrettmflynn
Copy link
Member

Hmm so this can't proceed past the File Metadata page because none of the unpacked values have a description or dtype—which is required in the table that this eventually is edited from.

Would there be a way to simply allow the actual metadata value to pass simply by setting the dtype to object for contact_vector? We are not editing the vector information itself in the Electrodes table (since it is ignored), so we don't really need to know the organization anyways.

Just to reiterate, the original problem was that the File Metadata would not validate because 'squaresomehow registered as invalid forcontact_shapesor somewhere deep incontact_vector`.

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Cody Baker <[email protected]>
@CodyCBakerPhD
Copy link
Collaborator Author

Hmm so this can't proceed past the File Metadata page because none of the unpacked values have a description or dtype

You should check out the metadata object passed to the electrode column table from the backend; they all have a description (though most are empty string "") and they do have dtype in the object but that's what I mentioned earlier about them not getting mapped onto the table right because most of them are in the numpy-style of "<{capital letter}{number of bytes}" so that mapping you have will need to be expanded

@garrettmflynn
Copy link
Member

Turns out nothing was showing up because data_type was expected rather than dtype. I've made some additional changes to handle the contact_vector information in the update_recording_properties_from_table_as_json—but I'm running into some issues.

  1. Completely removing contact_vector information from the columns and avoiding set_property for this particular value allows for moving forward with the conversion
  2. Including the original contact_vector dtype continues to throw the same error as before (Electrode helper functions #603 (comment))
  3. It's still unclear how correctly parsing the flattened contact_vector information back into the electrode_column_info for submission to set_property is supposed to change (2)

While your solution above does give us a way to edit contact_vector on the GUIDE, it doesn't fix the problem of submitting it back to NeuroConv particularly when using the default metadata value and schema information. If those defaults don't re-submit correctly, it's hard to tell what will and, therefore, how to edit and reconstruct successfully.

Can you advise on how to get the default contact_vector values to be resubmitted to NeurConv? Alternatively, as I've mentioned previously, we aren't actually allowing users to edit this value—so we could just omit any communication about this property.

@CodyCBakerPhD
Copy link
Collaborator Author

it doesn't fix the problem of submitting it back to NeuroConv

Yeah I didn't write anything on that - I'll take a look and modify the function to treat the fields like any other column

@CodyCBakerPhD
Copy link
Collaborator Author

Actually, on double-checking NeuroConv, we don't support the contact_vector property even if we passed/modified it

The problem I see is contact_shapes, but as for why it is in the electrode column at all I have no idea because if I look at the property keys on that recording extractor, it's not in the list at all. Any ideas? Was it hardcoded somewhere else? What ultimately triggers an error is the data type on it is int whereas it should be string (though it shouldn't be showing at all since it's a part of the contact vector subproperties)

BTW I also see a potential problem on my side with generating the tutorial data; no errors in the console to speak of, but no resulting folder/files show up in ~/NWB_GUIDE, nor does the file explorer pop up when done - would it be possible to add a file nav icon next to the tutorial generator that points to where it ended up?

@garrettmflynn
Copy link
Member

The problem I see is contact_shapes, but as for why it is in the electrode column at all I have no idea because if I look at the property keys on that recording extractor, it's not in the list at all. Any ideas? Was it hardcoded somewhere else? What ultimately triggers an error is the data type on it is int whereas it should be string (though it shouldn't be showing at all since it's a part of the contact vector subproperties)

Ah I see, I'm replicating that now.

Is it not included here (https://github.com/catalystneuro/neuroconv/blob/84ed29f014fdd2ed4702c37ae16f208346a467a8/src/neuroconv/datainterfaces/ecephys/spikeglx/spikeglx_utils.py#L25)?

@garrettmflynn
Copy link
Member

BTW I also see a potential problem on my side with generating the tutorial data; no errors in the console to speak of, but no resulting folder/files show up in ~/NWB_GUIDE, nor does the file explorer pop up when done - would it be possible to add a file nav icon next to the tutorial generator that points to where it ended up?

Is this still happening? Would best to see this in action.

I have a file nav icon when the generation is successful—though only the Generate button is displayed if the dataset has not been generated, it has been deleted, or (as you're seeing) it has failed.

Screenshot 2024-03-04 at 9 36 58 AM

In that case, do you want a file navigation icon to always be present to indicate where the data would end up?
Screenshot 2024-03-04 at 9 38 17 AM

@CodyCBakerPhD
Copy link
Collaborator Author

Is it not included here (https://github.com/catalystneuro/neuroconv/blob/84ed29f014fdd2ed4702c37ae16f208346a467a8/src/neuroconv/datainterfaces/ecephys/spikeglx/spikeglx_utils.py#L25)?

Hmm yes I suppose that one field technically is. Perhaps best to delay editing of it until generic editing of other contact vector subfields is too

@garrettmflynn
Copy link
Member

Got it. Ignoring this allows the conversion to go through.

@garrettmflynn
Copy link
Member

@CodyCBakerPhD Alright this should be good to review. Excited to see what you can break / uncover for additional improvements.

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review March 11, 2024 18:35
@CodyCBakerPhD CodyCBakerPhD merged commit b17e679 into ephys-metadata-v2 Mar 11, 2024
7 of 10 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the electrode_helpers branch March 11, 2024 18:35
@CodyCBakerPhD
Copy link
Collaborator Author

Merging and reviewing in #586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants