[ENH] Handle GLM designs with null or nan durations / onsets #3943

Remi-Gau · 2023-08-30T20:16:41Z

Closes [BUG] plot_event cannot handle events with 0 duration #3884

Changes proposed in this pull request:

add tests to make sure that events dataframe with nan onsets or durations throw errors and update code to make tests pass
add tests to throw warning when some events have null duration
update names of nilearn.glm.tests._utils public functions
refactor check_events from nilearn.glm.first_level into smaller functions
update example to show 0 duration events

github-actions · 2023-08-30T20:16:55Z

👋 @Remi-Gau Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

PR has an interpretable title.
PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
Code is PEP8-compliant (see our documentation on coding style)
Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

There is at least one unit test per new function / class (see our documentation on testing)
The new feature is demoed in at least one relevant example.

For bug fixes:

There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

Remi-Gau · 2023-08-30T20:29:21Z

to be merged after #3939 is merged

codecov · 2023-08-30T20:34:57Z

Codecov Report

Merging #3943 (42c71a8) into main (25e091a) will increase coverage by 0.03%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3943      +/-   ##
==========================================
+ Coverage   91.75%   91.79%   +0.03%     
==========================================
  Files         134      134              
  Lines       15751    15772      +21     
  Branches     3283     3284       +1     
==========================================
+ Hits        14453    14478      +25     
+ Misses        753      751       -2     
+ Partials      545      543       -2

Flag	Coverage Δ
macos-latest_3.10	`91.70% <100.00%> (+0.02%)`	⬆️
macos-latest_3.11	`91.70% <100.00%> (+0.02%)`	⬆️
macos-latest_3.8	`91.67% <100.00%> (+0.02%)`	⬆️
macos-latest_3.9	`91.67% <100.00%> (+0.02%)`	⬆️
ubuntu-latest_3.10	`91.70% <100.00%> (+0.02%)`	⬆️
ubuntu-latest_3.11	`?`
ubuntu-latest_3.8	`91.67% <100.00%> (+0.02%)`	⬆️
ubuntu-latest_3.9	`91.67% <100.00%> (+0.02%)`	⬆️
windows-latest_3.10	`91.64% <100.00%> (+0.02%)`	⬆️
windows-latest_3.11	`?`
windows-latest_3.8	`91.61% <100.00%> (+0.02%)`	⬆️
windows-latest_3.9	`91.61% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
nilearn/glm/first_level/experimental_paradigm.py	`100.00% <100.00%> (ø)`
nilearn/plotting/matrix_plotting.py	`93.61% <100.00%> (+0.11%)`	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Remi-Gau · 2023-09-02T15:24:12Z

from nilearn.plotting.matrix_plotting import (
    plot_event,
)

from nilearn.glm.tests._utils import design_with_null_durations
import matplotlib.pyplot as plt

plot_event(design_with_null_durations())
plt.show()

Before

After

If anyone has suggestion on how to test this, I am all ears.

bthirion

LGTM so far, thx.

bthirion · 2023-09-03T20:19:51Z

If anyone has suggestion on how to test this, I am all ears.

Maybe having this occur once in the doc is a good idea (rather than testing).

ymzayek

LGTM! While reviewing this, I was looking at plot_event function example in the docs and in the user guide there is a link to an example that shows its use but the wrong example is linked. It is a small change so maybe it can be done here.

This is referring to this section https://nilearn.github.io/dev/glm/first_level_model.html#event-based in the paragraph right after the bullet points where this example is linked Decoding of a dataset after GLM fit for signal extraction but it should be this one Generate an events.tsv file for the NeuroSpin localizer task

Remi-Gau · 2023-09-05T14:35:04Z

examples/04_glm_first_level/plot_write_events_file.py

@@ -19,27 +19,25 @@

 print(__doc__)

-#########################################################################
+# %%


implements @ymzayek suggestion: #3937

examples/04_glm_first_level/plot_write_events_file.py

Remi-Gau · 2023-09-05T14:37:04Z

LGTM! While reviewing this, I was looking at plot_event function example in the docs and in the user guide there is a link to an example that shows its use but the wrong example is linked. It is a small change so maybe it can be done here.

This is referring to this section https://nilearn.github.io/dev/glm/first_level_model.html#event-based in the paragraph right after the bullet points where this example is linked Decoding of a dataset after GLM fit for signal extraction but it should be this one Generate an events.tsv file for the NeuroSpin localizer task

Done. Thanks for catching this one.

examples/04_glm_first_level/plot_write_events_file.py

bthirion

Please add a whatsnew entry, even if this is a limited change.
Thx,

Remi-Gau · 2023-09-05T19:25:16Z

Please add a whatsnew entry, even if this is a limited change. Thx,

OK I was not 100% sure about this one. Will do.

Remi-Gau · 2023-09-06T01:04:23Z

doc/changes/latest.rst

@@ -54,6 +54,7 @@ Enhancements
 Changes
 -------

+- :func:`~glm.first_level.experimental_paradigm.check_events` will now throw a warning if some events have a 0 second duration and will throw an error if an event has ``NaN`` onset or duration (:gh:`3943` by `Rémi Gau`_).


:func:~glm.first_level.experimental_paradigm.check_events

and

:func:~glm.first_level.check_events

give me the same doc build error.

Am I missing something obvious?

check_events is not exposed in the docs so there is nothing to link to. I'm not sure if it is supposed to be part of the public API or not. If so, it would need to be added to doc/modules/glm.rst first before you can use that role

I wouldn't put it in the public API.

Then we just refer to it with the backticks without the func role

Wait... Just to make sure I understand.

What is the rationale of saying it is not in the public API but check_design_matrix is when both of them are found this init file?

https://github.com/nilearn/nilearn/blob/main/nilearn/glm/first_level/__init__.py#L24

As the codebase stands I wouldn't take what is in the init file for what is in the public API and supposed to be exposed to users since we know there is heterogeneity in the way "private" functions are written and also, we can have public API of a private module so that would be ok to include in the init. I agree though that given the refactoring we are doing regarding this, check_events should either be in a file named with a leading underscore or, since it is only used in design_matrix.py, it can be moved there and renamed with a leading underscore. I don't see that, following this convention, it would then be a problem if it is imported in a module's init file since it'll be clear from the import statement, which would then have a leading underscore, that it is private. This is what I understand from PEP8, though we may also want to make a decision on that aspect if it's confusing.

OK thanks for the clarification.

OK so for the moment it really feels the public API is whatever the doc days is in the public API. Hopefully things should get clearer once we clean things up.

For now I will use the backticks without the func role.

Remi-Gau added the Blocked label Aug 30, 2023

Remi-Gau mentioned this pull request Aug 30, 2023

[BUG] plot_event cannot handle events with 0 duration #3884

Closed

9 tasks

Remi-Gau removed the Blocked label Aug 31, 2023

ymzayek mentioned this pull request Sep 1, 2023

make sure public / private functions in _utils to start without / with a leading underscore #3628

Closed

Remi-Gau added 4 commits September 1, 2023 17:13

manual "rebase"

3c6cd72

make functions public

2967afb

throw errors for nan values

4a23b6b

refactor

a6b848a

Remi-Gau force-pushed the null_duration branch from 8aa3af7 to a6b848a Compare September 1, 2023 22:28

plot null duration events

1190043

bthirion reviewed Sep 3, 2023

View reviewed changes

ymzayek reviewed Sep 4, 2023

View reviewed changes

Remi-Gau added 2 commits September 5, 2023 10:14

fix link in doc

3b773c4

update example

f8500a1

Remi-Gau commented Sep 5, 2023

View reviewed changes

examples/04_glm_first_level/plot_write_events_file.py Show resolved Hide resolved

Remi-Gau added 2 commits September 5, 2023 10:40

ignore results

75c031c

revert useless change

26a793d

Remi-Gau marked this pull request as ready for review September 5, 2023 14:52

ymzayek reviewed Sep 5, 2023

View reviewed changes

examples/04_glm_first_level/plot_write_events_file.py Outdated Show resolved Hide resolved

rm useless code

2422c55

bthirion approved these changes Sep 5, 2023

View reviewed changes

update changelog

fa72f75

Remi-Gau commented Sep 6, 2023

View reviewed changes

rm :func: from changelog

42c71a8

Remi-Gau merged commit d4690b9 into nilearn:main Sep 6, 2023
28 of 29 checks passed

Remi-Gau deleted the null_duration branch September 6, 2023 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Handle GLM designs with null or nan durations / onsets #3943

[ENH] Handle GLM designs with null or nan durations / onsets #3943

Remi-Gau commented Aug 30, 2023 •

edited

github-actions bot commented Aug 30, 2023 •

edited by Remi-Gau

Remi-Gau commented Aug 30, 2023

codecov bot commented Aug 30, 2023 •

edited

Remi-Gau commented Sep 2, 2023

bthirion left a comment

bthirion commented Sep 3, 2023

ymzayek left a comment

Remi-Gau Sep 5, 2023

Remi-Gau commented Sep 5, 2023

bthirion left a comment

Remi-Gau commented Sep 5, 2023

Remi-Gau Sep 6, 2023 •

edited

ymzayek Sep 6, 2023

bthirion Sep 6, 2023

ymzayek Sep 6, 2023

Remi-Gau Sep 6, 2023

ymzayek Sep 6, 2023

Remi-Gau Sep 6, 2023

[ENH] Handle GLM designs with null or nan durations / onsets #3943

[ENH] Handle GLM designs with null or nan durations / onsets #3943

Conversation

Remi-Gau commented Aug 30, 2023 • edited

github-actions bot commented Aug 30, 2023 • edited by Remi-Gau

Remi-Gau commented Aug 30, 2023

codecov bot commented Aug 30, 2023 • edited

Codecov Report

Remi-Gau commented Sep 2, 2023

Before

After

bthirion left a comment

Choose a reason for hiding this comment

bthirion commented Sep 3, 2023

ymzayek left a comment

Choose a reason for hiding this comment

Remi-Gau Sep 5, 2023

Choose a reason for hiding this comment

Remi-Gau commented Sep 5, 2023

bthirion left a comment

Choose a reason for hiding this comment

Remi-Gau commented Sep 5, 2023

Remi-Gau Sep 6, 2023 • edited

Choose a reason for hiding this comment

ymzayek Sep 6, 2023

Choose a reason for hiding this comment

bthirion Sep 6, 2023

Choose a reason for hiding this comment

ymzayek Sep 6, 2023

Choose a reason for hiding this comment

Remi-Gau Sep 6, 2023

Choose a reason for hiding this comment

ymzayek Sep 6, 2023

Choose a reason for hiding this comment

Remi-Gau Sep 6, 2023

Choose a reason for hiding this comment

Remi-Gau commented Aug 30, 2023 •

edited

github-actions bot commented Aug 30, 2023 •

edited by Remi-Gau

codecov bot commented Aug 30, 2023 •

edited

Remi-Gau Sep 6, 2023 •

edited