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

Time alignment #762

Merged
merged 47 commits into from
May 30, 2024
Merged

Time alignment #762

merged 47 commits into from
May 30, 2024

Conversation

garrettmflynn
Copy link
Member

This PR implements time alignment.

@CodyCBakerPhD Would be great to get your help on a few final things:

  1. Can you point me to an example for setting up a Dummy Recording for a sorting interface so that we can simply set timestamps / starting time?
  2. The current iteration isn't properly setting timestamps on the preview conversion. Is there a reason this is happening?

Besides the above caveats, this is properly updating the time alignment information and requesting it back on the Source Data popup.

@garrettmflynn garrettmflynn self-assigned this Apr 22, 2024
@CodyCBakerPhD
Copy link
Collaborator

Can you point me to an example for setting up a Dummy Recording for a sorting interface so that we can simply set timestamps / starting time?

Not a 'dummy' example, but a proper test case from example data: https://github.com/catalystneuro/neuroconv/blob/179711773fc67a2a733ebc6522afa0bea6f2f305/tests/test_on_data/test_sorting_interfaces.py#L29-L33

https://github.com/catalystneuro/neuroconv/blob/179711773fc67a2a733ebc6522afa0bea6f2f305/src/neuroconv/tools/testing/data_interface_mixins.py#L586-L589

Easy way to truly do a 'dummy' one is the use the official Mock one: https://github.com/catalystneuro/neuroconv/blob/179711773fc67a2a733ebc6522afa0bea6f2f305/tests/test_ecephys/test_ecephys_interfaces.py#L127-L131

The current iteration isn't properly setting timestamps on the preview conversion. Is there a reason this is happening?

I'll look into it

@garrettmflynn
Copy link
Member Author

And just to confirm, we actually do want to allow users to specify timestamps or a start time for Sorting Interfaces, independent of any active Recording Interfaces?

Mentioned this to Ben at the developer hackathon and he was a bit confused about what this actually would mean.

@CodyCBakerPhD
Copy link
Collaborator

If the exact recording interface used to generate the sorting is present in the pipeline, then they should link the sorting to the recording and adjust timestamps or shift starting time of the recording

Otherwise, they still need to be able to set timestamps and shift starting time as if they had chosen to include the original recording

@garrettmflynn
Copy link
Member Author

Ah gotcha. Makes sense now

@garrettmflynn
Copy link
Member Author

So this is practically ready for review.

Now that I've expanded my tests beyond the SpikeGLX-Phy test case, though, I've realized that the Phy Sorting Interface in the tutorial data seems to be locked to the SpikeGLX Recording Interface regardless of any user-defined parameters. Is this an expected behavior?

Screenshot 2024-04-24 at 2 44 37 PM

This was the previous behavior before https://github.com/catalystneuro/neuroconv/pull/823—though an error was thrown about the None and int addition. If providing a mock recording interface, the specified start time would take effect but it wasn't clear what it was shifting.

@garrettmflynn
Copy link
Member Author

Also confirmed that this time offset on the Recording Interface does not show up in stub files but can be seen from Neurosift after a full conversion.

@CodyCBakerPhD
Copy link
Collaborator

A couple things I noticed testing this

i) the option of linking between compatible sorting and recording is now failing for the tutorial data (no button shows up)

ii) I set timestamps for Suite2p (single plane; 250 frames); they seem to have 'uploaded' to the interface fine because the alignment bar did say the proper start and stop interval according to the timestamps; but then they do not show up even in the final converted file

iii) Nothing seems to be applying to stub mode, I'm investigating that one myself

@garrettmflynn
Copy link
Member Author

garrettmflynn commented May 30, 2024

Updated so that (i) works! Can you try again? Would be interested to get the file you're using too so I can replicate.

  • When I do (i), I am getting an error during the conversion—but I can successfully associate the two.

Just FYI I was able to use the Suite2P test pipeline and align to an arbitrary start time, which I could observe across stub and full conversions.

Comment on lines 735 to 736
except Exception as e:
errors[name] = str(e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm almost done refactoring a bit here and there on the backend for this

But how did you want the frontend to handle this error step given the recent advances in logging?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I.e., can we just let them raise?

Copy link
Member Author

@garrettmflynn garrettmflynn May 30, 2024

Choose a reason for hiding this comment

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

So the reason for this is to render errors occurring in the Time Alignment pop-up next to their respective interfaces. If uncaught, you'd have to fail the entire request. This allows us to limit failure to the impacted interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whether and how to log these errors in the log file is a separate—and more interesting—question. Is that the desired outcome for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm.. I see

OK we'll tackle that in a follow-up. Would probably opt to dump the list of compiled errors all at once

@CodyCBakerPhD
Copy link
Collaborator

FYI the issue I had with timestamps was a non-issue; the timestamps vector I was trying to set was regular, and NeuroConv magic automatically detected that and set to a regular rate. There's ongoing discussions over there about whether that should even happen at all though

@CodyCBakerPhD
Copy link
Collaborator

Also confirmed the issue with the recording interfaces in stub mode is on the NeuroConv side; will push a fix for that in the future so shouldn't hold up this PR

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge May 30, 2024 22:15
@CodyCBakerPhD CodyCBakerPhD merged commit 06488b6 into main May 30, 2024
23 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the time-alignment-ui branch May 30, 2024 22:34
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