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

Process: Add combine recordings #717

Merged
merged 35 commits into from
Nov 22, 2024

Conversation

Edouard2laire
Copy link
Contributor

@Edouard2laire Edouard2laire commented Jul 6, 2024

Hello,

This is the continuation of #660 in the attempt to enhance multimodal data analysis in Brainstorn.

Since, having multiple raw viewer recording with multiple sampling rate is not accessible (#656), i made this process to combine recording and resampling them to the highest frequency.

This allows to make such visualization directly from Brainstorm:

image

This assume that the signals were synchronized

Todo:

  • User-specified folder name
  • Copy channel flag
  • Copy video if present (not necessary actually. can be loaded from the other folder)
  • Events are concatenated, they are channel-wise for the channels in their original file
  • Channel locations are merged, they are already SCS
  • Projectors are also merged

Question:
- [x] Better way to merge channels?
I am not sure if the way I am doing is ok; especially if I need to merge headpoints / fiducials ...

- [x] How to deal with events ?
Same thing, I am only copying events from the first file ? Should we merge them adding a sufixe to the name to identify the file ?

@Edouard2laire Edouard2laire marked this pull request as ready for review September 5, 2024 18:20
@Edouard2laire
Copy link
Contributor Author

hello @rcassani

This PR is ready for review.

The only things I am not sure are: the events ( how to merge event from multiple file - esp after synchronization there will be a lot of duplicate if we just concatenate the events), and the channel flag.

Let me know if you have any questions.

I'll try to complete all the PR I have as draft soon :)

@Edouard2laire
Copy link
Contributor Author

ok i completed my todo list.

@rcassani, let me know when you would have time to review this. Let me know if you need an example dataset

@rcassani rcassani closed this Oct 30, 2024
@rcassani rcassani reopened this Oct 30, 2024
@rcassani
Copy link
Member

@Edouard2laire, apologies for closing the PR, I was trying to close my comment —duh!

This will be added in master
@rcassani
Copy link
Member

@Edouard2laire, my changes are done, thanks for the patience.
Please test them, then we can merge it

@Edouard2laire
Copy link
Contributor Author

Thanks a lot. I am starting the test now.

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 21, 2024

it seems to be working.

However, i am getting this error a lot on the data of PA14 that I sent you:

image

is this something I should worry about?

It seems to be present at the end of the file:

image

But when I look at the nirs file at the same time it looks like this:

image

So the individual files are from 0 to 7056.855, 7056.9 and 7056.856 but the output is 7078.996

I am wondering why 22 sec were added at the end of the file.

@Edouard2laire
Copy link
Contributor Author

@rcassani so it was an issue with how my file was imported. Reimported from raw file and it is now working !
Fixed a small bug for the video.

Ready to merge :)

@Edouard2laire
Copy link
Contributor Author

Actually wait. i am trying something for the events :)

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 21, 2024

I made some change on the fusion of the events to avoid having multiple copy of the same events, or multiple events with the same name.

Let me know if that is ok for you.

Here is the output after the change:
image

@Edouard2laire Edouard2laire changed the title Combine recordings Process: Add combine recordings Nov 22, 2024
@rcassani
Copy link
Member

@Edouard2laire, thanks for noticing the part for events with same name.

Don't you think it is necessary to keep the events separated per file?

If we merge the events at combining recordings, it will not possible to split them after. Also, it is possible that events with the same name are different type (single / extended) in that case merging is not even possible. On the other hand, if we keep the events separated in the combined file, the user can easily decide to merge them. This is done with the process_evt_merge.m or in the GUI (which calls the process), and this process will handle the cases when there is same name, different types, merging if same time sample, etc.

This could be implementing by reverting to 54d1cb4 and this patch:

diff --git a/toolbox/process/functions/process_combine_recordings.m b/toolbox/process/functions/process_combine_recordings.m
index 3926c2b9..37bc1b1f 100644
--- a/toolbox/process/functions/process_combine_recordings.m
+++ b/toolbox/process/functions/process_combine_recordings.m
@@ -135,6 +135,8 @@ function OutputFiles = Run(sProcess, sInputs)
         % Concatenate events
         for iEvent = 1 : length(sMetaData(iInput).F.events)
             tmpEvent = sMetaData(iInput).F.events(iEvent);
+            % Rename event if duplicated
+            tmpEvent.label = file_unique(tmpEvent.label, {NewEvents.label});
             % Add channel info
             addedChannelNames = {NewChannelMat.Channel(sIdxChNew{iInput}).Name};
             nOccurences = size(tmpEvent.times, 2);

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 22, 2024

@Edouard2laire, thanks for noticing the part for events with same name.

Don't you think it is necessary to keep the events separated per file?

I don't see why it is necessary. At least I think it is important not to duplicate identical events. (since we duplicate them during the synchronization) it would lead to so many events in the combined file.

If we merge the events at combining recordings, it will not possible to split them after.

First, i dont think this is a good idea to split a combine file: we had to upsample a lot the recording. so splitting the signal again doesnt make too much sense. The combining is only here for visualization since we cant visualize multiple signal with different frequency.

But even then, it is easy to split: If an event has no channel information:it belong to all channel and can be split easily. just copy to each file.
If an event has channel information, you can attribute to file based on the channel

Also, it is possible that events with the same name are different type (single / extended) in that case merging is not even possible. On the other hand, if we keep the events separated in the combined file, the user can easily decide to merge them. This is done with the process_evt_merge.m or in the GUI (which calls the process), and this process will handle the cases when there is same name, different types, merging if same time sample, etc.

I agree that we can keep the events that are not identical but have the same name and not merge them.
Btw, i copied the code from the GUI and it doesnt seems it is calling the process : https://github.com/brainstorm-tools/brainstorm3/blob/master/toolbox/gui/panel_record.m#L1963-L2029

@Edouard2laire
Copy link
Contributor Author

Edouard2laire commented Nov 22, 2024

Would this work for you ? @rcassani

in this case, we end up with the following events:

image

and yes we can merge the motion latter using the GUI or process

@rcassani
Copy link
Member

I don't see why it is necessary. At least I think it is important not to duplicate identical events. (since we duplicate them during the synchronization) it would lead to so many events in the combined file.

Makes sense for the event that was used for synchronization.

But even then, it is easy to split: If an event has no channel information:it belong to all channel and can be split easily. just copy to each file.
If an event has channel information, you can attribute to file based on the channel

Yes, this is true. But there is no way to separate events based on the channels that the belong in the GUI or with a process. The user would need to program it. But by not merging (in the combining process), the user can decide to merge them, which can be done with the GUI and there is already a process.

Btw, i copied the code from the GUI and it doesnt seems it is calling the process : https://github.com/brainstorm-tools/brainstorm3/blob/master/toolbox/gui/panel_record.m#L1963-L2029

Oh! I'll add this on my TODO list, outside of this PR. Thanks for the heads-up

@rcassani
Copy link
Member

and yes we can merge the motion latter using the GUI or process

Sounds good

@rcassani
Copy link
Member

@Edouard2laire, two changes in 83b10d0:

  1. With your recent changes, if an event was not channel-wise and was present in only on file. Once combined, this event was also not channel-wise, so it suggested that it applied to all channels, not only for channels of the file that had the event. This is now fixed.

  2. The fact that duplicate names started in _02 was bugging me. Now duplicated events are _01, ...
    So the intuitive thing to do at the moment of merging will be to set the event label to the name without the numbers.

@Edouard2laire
Copy link
Contributor Author

The fact that duplicate names started in _02 was bugging me. Now duplicated events are _01, ...
So the intuitive thing to do at the moment of merging will be to set the event label to the name without the numbers.

yes, me too. but i was not sure on the best way to fix that :) Glad you have done it :)

@rcassani rcassani merged commit ba154f9 into brainstorm-tools:master Nov 22, 2024
@Edouard2laire Edouard2laire deleted the combine-recordings branch November 22, 2024 22:10
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