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

handling of simultaneous events by plot_event #3948

Closed
Remi-Gau opened this issue Sep 2, 2023 · 4 comments · Fixed by #3994
Closed

handling of simultaneous events by plot_event #3948

Remi-Gau opened this issue Sep 2, 2023 · 4 comments · Fixed by #3994
Assignees

Comments

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Sep 2, 2023

Other "problems" with plot_event that I am encountering while working on #3884.

  1. if 2 events have different trial_type but same onset and duration, then only the last one plotted is visible as it is overlayed over the other one

  2. design matrix creation will sum the amplitude of 2 "duplicated" events (same trial type, onset, duration), however this won't be apparent when just using the plot_event function.

import matplotlib.pyplot as plt
import numpy as np

from nilearn.glm.first_level import make_first_level_design_matrix
from nilearn.glm.tests._utils import duplicate_events_paradigm
from nilearn.plotting import plot_design_matrix, plot_event
from nilearn.plotting.matrix_plotting import plot_event

events = duplicate_events_paradigm()
events.drop(columns=["modulation"], inplace=True)
print(events)

plot_event(events)
plt.show()

fig, (ax1, ax2) = plt.subplots(figsize=(10, 6), nrows=1, ncols=2)

tr = 1.0
n_scans = 80
frame_times = np.arange(n_scans) * tr

X1 = make_first_level_design_matrix(
    frame_times,
    events,
    drift_model="polynomial",
    drift_order=3,
    hrf_model="spm",
)

plot_design_matrix(X1, ax=ax1)
ax1.set_title("design matrix with duplicated events", fontsize=12)

plt.plot(X1["c0"])
plt.plot(X1["c1"])
ax2.set_title("predicted time course of 'c0' and 'c1'", fontsize=12)

plt.show()
  trial_type  onset  duration
0         c0     10       1.0
1         c0     30       1.0
2         c0     70       1.0
3         c0     70       1.0  < -- duplicated events
4         c1     10       1.0  < -- "hides" similar events from c0
5         c1     30       1.0  < -- "hides" similar events from c0

events

Figure_1

(1) Could be solved but may require plotting events on several "rows" (one per condition).

(2) May be the intended behavior, but if not this could probably be easily solved by running check_events on the input of plot_event (which may also help with some of the input validation that is missing from this function in anycase: for example, no checking that the required columns are there, do not have NaNs...)

Notes:

  • I do find it confusing that a function called check_events takes care of summing the modulation of the events duplicates: this behavior should probably extracted in a separate function

  • although a warning is sent when events duplicates are detected, it may be more user friendly to have the possibility to drop duplicated events rather than sum their modulation

@bthirion
Copy link
Member

bthirion commented Sep 3, 2023

(1) Could be solved but may require plotting events on several "rows" (one per condition).

Indeed. But is this really needed ? I guess that people avoid having simultaneous events in general ? Just worried about yagni

(2) May be the intended behavior, but if not this could probably be easily solved by running check_events on the input of plot_event (which may also help with some of the input validation that is missing from this function in anycase: for example, no checking that the required columns are there, do not have NaNs...)

Could be a good idea indeed.

@bthirion
Copy link
Member

bthirion commented Sep 3, 2023

  • I do find it confusing that a function called check_events takes care of summing the modulation of the events duplicates: this behavior should probably extracted in a separate function

Yes

@bthirion
Copy link
Member

bthirion commented Sep 3, 2023

  • although a warning is sent when events duplicates are detected, it may be more user friendly to have the possibility to drop duplicated events rather than sum their modulation

If this needed(?)

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Sep 5, 2023

Indeed. But is this really needed ? I guess that people avoid having simultaneous events in general ? Just worried about yagni

Agreed that this falls in the overkill category and that most events file in fmri are (unfortunately) not as rich as those from EEG.
Still think that throwing a warning may be a good thing.

running check_events on the input of plot_event

Could be a good idea indeed.

Will PR this after #3884 to keep PR small

summing the modulation of the events duplicates: this behavior should probably extracted in a separate function

Probably another small PR

  • possibility to drop duplicated events rather than sum their modulation

If this needed(?)

probably not unless users ask for it

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 a pull request may close this issue.

2 participants