[ENH] implement logger for first_level_from_bids #3702

Remi-Gau · 2023-04-18T14:00:08Z

Changes proposed in this pull request:

shows a proof of concept to implement a logger for first_level_from_bids

Remi-Gau · 2023-04-18T14:01:45Z

nilearn/glm/first_level/first_level.py

+logging.basicConfig(level=logging.ERROR)
+logger = logging.getLogger(name="nilearn")


potentially create a function nilearn/_utils/logger.py ?

actually there is one 😆 it's used in a few modules but not consistently. it should probably be removed as part of a larger change if we replace current print calls with logging

Remi-Gau · 2023-04-18T14:02:27Z

nilearn/glm/first_level/first_level.py

@@ -1527,8 +1511,7 @@ def _make_bids_files_filter(
    if extra_filter and supported_filters:
        for filter_ in extra_filter:
            if filter_[0] not in supported_filters:
-                if verbose:
-                    warn(
+                logger.warning(


should warning be handled by the logger as well?

IIUC the warning reports an error that the application developer should correct, so IMO and my understanding of the logging module's documentation it should be a warning (not a logging event), and wether we use logging or not it shouldn't be controlled by the verbose parameter

github-actions · 2023-04-18T14:11:22Z

👋 @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.

codecov · 2023-04-18T14:12:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9a50716) 91.40% compared to head (2e29053) 91.38%.
Report is 368 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3702      +/-   ##
==========================================
- Coverage   91.40%   91.38%   -0.02%     
==========================================
  Files         133      133              
  Lines       15536    15552      +16     
  Branches     3225     3225              
==========================================
+ Hits        14200    14212      +12     
- Misses        784      787       +3     
- Partials      552      553       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Remi-Gau · 2023-04-18T14:21:26Z

@jeromedockes this is just a quick dirty example of how things could look like with a logger.

Let me know what you think.

jeromedockes

sorry I didn't realise implement a logger meant start using the logging standard library module. I think it's a good idea, but then we would probably want to use it consistently throughout the library and plan it a bit more... there is some discussion to be had as some people find logging confusing, and we should read through related scikit-learn discussions (linked below).

and maybe wait for the formatting stuff to be over so we don't have too many widespread changes going on at the same time

for the specific case of 1st level from bids, I wonder if we should have a class to have fewer functions with a lot of parameters, and a way to store some info like the discovered files and produce reports?

jeromedockes · 2023-04-19T19:26:54Z

nilearn/glm/first_level/first_level.py

+logging.basicConfig(level=logging.ERROR)
+logger = logging.getLogger(name="nilearn")


actually there is one 😆 it's used in a few modules but not consistently. it should probably be removed as part of a larger change if we replace current print calls with logging

jeromedockes · 2023-04-19T19:33:14Z

nilearn/glm/first_level/first_level.py

@@ -49,6 +49,11 @@
 from sklearn.cluster import KMeans


+import logging
+
+logging.basicConfig(level=logging.ERROR)


in theory libraries are not supposed to mess with the logging configuration, and definitely not add any handlers to the root logger (doc), as that is left up to the application... I guess current joblib/scikit-learn/nilearn approach of verbose and print is quite far from the standard library logging module's philosophy, and it might be worth reading the related discussions in related scikit-learn issues: roadmap, 6929, 78

also the default handlers writes to stderr while the rest of nilearn messages made with print would go to stdout, it would be a bit weird to have just the first_level_model_from_bids output go to a different stream

jeromedockes · 2023-04-19T19:42:09Z

nilearn/glm/first_level/first_level.py

@@ -1154,12 +1161,12 @@ def _report_found_files(
        Only one filter per field allowed.

    """
-    print(


is this the only thing that is controlled by the verbose parameter in our case?

jeromedockes · 2023-04-19T19:45:37Z

nilearn/glm/first_level/first_level.py

@@ -1527,8 +1511,7 @@ def _make_bids_files_filter(
    if extra_filter and supported_filters:
        for filter_ in extra_filter:
            if filter_[0] not in supported_filters:
-                if verbose:
-                    warn(
+                logger.warning(


IIUC the warning reports an error that the application developer should correct, so IMO and my understanding of the logging module's documentation it should be a warning (not a logging event), and wether we use logging or not it shouldn't be controlled by the verbose parameter

Remi-Gau · 2023-04-20T07:54:16Z

sorry I didn't realise implement a logger meant start using the logging standard library module. I think it's a good idea, but then we would probably want to use it consistently throughout the library and plan it a bit more...

yeah the "should we use a more systematic and planned approach to this?" was in the back of my mind and hence why I prefer to draft things here to get early feedback.

Do you think I should maybe rename the issue #3686 to reflect a potential bigger scope for this?

there is some discussion to be had as some people find logging confusing, and we should read through related scikit-learn discussions (linked below).

thanks: will have a look

and maybe wait for the formatting stuff to be over so we don't have too many widespread changes going on at the same time

Agreed

for the specific case of 1st level from bids, I wonder if we should have a class to have fewer functions with a lot of parameters, and a way to store some info like the discovered files and produce reports?

Will look into this as well.

implement draft logger

2e29053

Remi-Gau commented Apr 18, 2023

View reviewed changes

jeromedockes reviewed Apr 19, 2023

View reviewed changes

Remi-Gau added the GLM Issues/PRs related to the nilearn.glm module. label Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] implement logger for first_level_from_bids #3702

[ENH] implement logger for first_level_from_bids #3702

Remi-Gau commented Apr 18, 2023

Remi-Gau Apr 18, 2023

jeromedockes Apr 19, 2023

Remi-Gau Apr 18, 2023

jeromedockes Apr 19, 2023

github-actions bot commented Apr 18, 2023 •

edited by Remi-Gau

codecov bot commented Apr 18, 2023 •

edited

Remi-Gau commented Apr 18, 2023

jeromedockes left a comment

jeromedockes Apr 19, 2023

jeromedockes Apr 19, 2023

jeromedockes Apr 19, 2023

jeromedockes Apr 19, 2023

jeromedockes Apr 19, 2023

Remi-Gau commented Apr 20, 2023

		logging.basicConfig(level=logging.ERROR)
		logger = logging.getLogger(name="nilearn")

[ENH] implement logger for first_level_from_bids #3702

Are you sure you want to change the base?

[ENH] implement logger for first_level_from_bids #3702

Conversation

Remi-Gau commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2023 • edited by Remi-Gau

codecov bot commented Apr 18, 2023 • edited

Codecov Report

Remi-Gau commented Apr 18, 2023

jeromedockes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Apr 20, 2023

github-actions bot commented Apr 18, 2023 •

edited by Remi-Gau

codecov bot commented Apr 18, 2023 •

edited