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

Render Units Table #679

Merged
merged 46 commits into from
Mar 29, 2024
Merged

Render Units Table #679

merged 46 commits into from
Mar 29, 2024

Conversation

garrettmflynn
Copy link
Member

This PR allows you to view the units table. Currently, editing is disabled for all related properties to simplify the user interactions with the table to editing unit properties outside the GUIDE.

@garrettmflynn garrettmflynn self-assigned this Mar 15, 2024
@CodyCBakerPhD
Copy link
Collaborator

Attempting this on dataset_3 of cellexplorer yields

Traceback (most recent call last):
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\app.py", line 1484, in full_dispatch_request
    rv = self.dispatch_request()
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\app.py", line 1469, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\api.py", line 404, in wrapper
    resp = resource(*args, **kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask\views.py", line 109, in view
    return current_app.ensure_sync(self.dispatch_request)(**kwargs)
  File "D:\anaconda3\envs\nwb-guide\lib\site-packages\flask_restx\resource.py", line 46, in dispatch_request
    resp = meth(*args, **kwargs)
  File "D:\GitHub\nwb-guide\pyflask\apis\neuroconv.py", line 97, in post
    return get_metadata_schema(neuroconv_api.payload.get("source_data"), neuroconv_api.payload.get("interfaces"))
  File "D:\GitHub\nwb-guide\pyflask\manageNeuroconv\manage_neuroconv.py", line 361, in get_metadata_schema
    schema["properties"]["Ecephys"]["required"].append("Electrodes")
KeyError: 'required'

@CodyCBakerPhD
Copy link
Collaborator

Default description for brain_area should replace 'electrode' with 'unit'

UnitColumns description should be editable so I can change it from "No description"

Since we're not allowing direct editing of table content, can disallow 'data type' from unit column table

The more I think about it the more I think this section should just be for control over unit table column descriptions; in your opinion, is it even useful to render the values of those columns on the page if they can't be easily controlled? This is really more of a behavior relegated to the 'preview' pages

@garrettmflynn
Copy link
Member Author

IMO it might still be useful to see the data to provide an appropriate column description—otherwise I think you're right in framing the purpose of the Units information this way.

@garrettmflynn
Copy link
Member Author

Updated!

BTW I realized now that we're never setting the new descriptions in NeuroConv. How would I actually do this?

@CodyCBakerPhD
Copy link
Collaborator

BTW I realized now that we're never setting the new descriptions in NeuroConv. How would I actually do this?

That is the one thing here adjusted via official metadata (for the electrode table too in case it wasn't previously)

So you get the defaults with metadata = converter.get_metadata() and you modify that dict, and pass that dict back to `converter.run_conversion(..., metadata=metadata)

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Mar 18, 2024

How should we reconcile the fact that there could be multiple ElectrodeColumns/UnitColumns tables across different interfaces, each with their own values for this?

Since as far as I remember, the metadata squashes everything down to a single table.

@CodyCBakerPhD
Copy link
Collaborator

How should we reconcile the fact that there could be multiple ElectrodeColumns/UnitColumns tables across different interfaces, each with their own values for this?

Two options:

(i) pull individual metadata per interface, which mimics the electrode/unit table values due to the one-to-one object mapping. That is,

interface.get_metadata()

but this would then require you to (a) validate all descriptions across all tables to ensure they are consistent, and (b) take a union of all the consistent column definitions to send back to the converter

This has the benefit then of each column table having a more direct relation to it's nearest electrode table, removing and confusion about mismatches between the two

(ii) Move the column definition table to the top of all and have it already start as what the converter.get_metadata() gives you

This has the benefit that there's no duplication or multiple validation steps to check for consistency. The top table would define all columns and their definitions/data types regardless of what columns a particular electrode or unit table might have

@garrettmflynn
Copy link
Member Author

(2) is going to be the least confusing to implement and interact with, but would also result in the inability to add new interface-specific columns.

Is this alright? Otherwise, we could also declare an additional property (e.g. interfaces) that indicates the interface(s) that any particular column applies to—and which defaults to all if blank.

@CodyCBakerPhD
Copy link
Collaborator

new interface-specific columns.

Well, you would add them to the global column definitions for all electrodes tables and then maybe add a new column with values on certain particular subtables, right?

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Mar 18, 2024

Starting to work with both a global table and subtables will contain the same challenges as (1) involving some reconciliation of duplicated columns.

So I'd rather avoid that if possible.

@garrettmflynn
Copy link
Member Author

Gotcha. Fixed!

@CodyCBakerPhD
Copy link
Collaborator

OK thanks, that looks better now

The unit column table doesn't seem to be saving any of my changes to the descriptions though; even if I hit 'save' at the top, if I proceed to the next page then come back it restored to the default. Any ideas?

@garrettmflynn
Copy link
Member Author

Weird I don't have the same problem.

Screenshot 2024-03-25 at 1 23 33 PM

I've updated the previous "name" column to "unit_name" to see if that description would update—but it seems like I'm doing that wrong or that isn't possible.

@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn Small conflict here

@garrettmflynn
Copy link
Member Author

Still haven't gotten around to those PC-specific issues with the description that we noticed—but the rest should be reviewable.

@CodyCBakerPhD
Copy link
Collaborator

Are you able to reproduce them locally?

Also, failing tests on this branch

@garrettmflynn
Copy link
Member Author

Running into some issues on my PC. Updated the environment and ensured that I have the same testing data as my Mac, but this error is being thrown when going off the Source Data page of the SpikeGLX-Phy test pipeline.
Screenshot 2024-03-28 142220

@CodyCBakerPhD
Copy link
Collaborator

versions of neo and SI?

@CodyCBakerPhD
Copy link
Collaborator

Otherwise maybe redownload the files from S3 or otherwise make sure you're pointing to the right files. The nSavedChannels is definitely there

@garrettmflynn
Copy link
Member Author

spikeinterface is at 0.100.3, while neo is 0.13.0. I'll re-download the data and try again, as this is using the test pipeline without modifications (which works fine elsewhere) and manually re-specifying the source data didn't work.

It's also quite odd because the Exclude Cluster Groups option for Phy is back to being improperly specified and flagged with that yellow warning.

Screenshot 2024-03-28 163107

@garrettmflynn
Copy link
Member Author

Redownloading fixed the Source Data issue. But for some reason the Ecephys metadata is excluded from the schema returned from the GUIDE server, and the returned Ecephys appears to be unprocessed (i.e. before we updated to include electrode data, even).

I'm on the units-table branch and have swapped several times. The server runs appropriately after running npm start.

Just sharing because it's such weird behavior and I have no idea where to start to debug. If you have any thoughts, let me know.

If nothing comes to mind, though, don't worry about it. I'll slog through it as soon as I can.

@CodyCBakerPhD
Copy link
Collaborator

Just sharing because it's such weird behavior and I have no idea where to start to debug. If you have any thoughts, let me know.

In these events I don't just do a complete environment wipe but also a complete local git wipe

IDK if related, but above I did find it weird to see you using the classic command shell instead of Anaconda prompt

@garrettmflynn
Copy link
Member Author

Okay I will try that.

Oddly, running NPM start changes how my Anaconda prompt tab renders to appear like a classic command prompt. Though I tested again this morning and ensured that I used the Anaconda prompt, but this does not change the behavior.

@garrettmflynn
Copy link
Member Author

The full Git wipe worked! Thanks for sticking with me on this lol.

Definitely less acquainted with common Windows issues than I should be.

@garrettmflynn
Copy link
Member Author

However, I'm not able to replicate the description auto-update and save issue you've been seeing.

Specifically, I tested this on the SpikeGLX-Phy test pipeline and updated the channel_name description.

Can you try again on the latest push and see if you get the same result as before?

@CodyCBakerPhD
Copy link
Collaborator

I too did a complete wipe (including NWB_GUIDE home directory)

The original invalidation issues are indeed gone now, but made a video of the description thing

video1603566724.mp4

It's pretty minor though, I'm OK with tabling as a low priority thing to figure out in a followup if it's too hard to pin down

@CodyCBakerPhD CodyCBakerPhD merged commit 71b53d8 into main Mar 29, 2024
14 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the units-table branch March 29, 2024 17:50
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