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.

Sign up for GitHub

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

Jump to bottom

[ENH] implement logger for first_level_from_bids #3702

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[ENH] implement logger for first_level_from_bids #3702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
93 changes: 37 additions & 56 deletions nilearn/glm/first_level/first_level.py
View file
Open in desktop
Expand Up @@ -49,6 +49,11 @@
from sklearn.cluster import KMeans


import logging

logging.basicConfig(level=logging.ERROR)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

logger = logging.getLogger(name="nilearn")
Comment on lines +54 to +55
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


def mean_scaling(Y, axis=0):
"""Scaling of the data to have percent of baseline change \
along the specified axis.
Expand Down Expand Up @@ -928,6 +933,14 @@ def first_level_from_bids(dataset_path,
Items for the FirstLevelModel fit function of their respective model.

"""

if verbose == 1:
logger.setLevel(logging.WARNING)
elif verbose == 2:
logger.setLevel(logging.INFO)
elif verbose == 3:
logger.setLevel(logging.DEBUG)

if slice_time_ref == 0:
warn(
'Starting in version 0.12, slice_time_ref will default to None.',
Expand Down Expand Up @@ -962,7 +975,6 @@ def first_level_from_bids(dataset_path,
supported_filters=[*_bids_entities()["raw"],
*_bids_entities()["derivatives"]],
extra_filter=img_filters,
verbose=verbose
)
inferred_t_r = _infer_repetition_time_from_dataset(
bids_path=derivatives_path,
Expand All @@ -973,7 +985,6 @@ def first_level_from_bids(dataset_path,
task_label=task_label,
supported_filters=[*_bids_entities()["raw"]],
extra_filter=img_filters,
verbose=verbose
)
inferred_t_r = _infer_repetition_time_from_dataset(
bids_path=dataset_path,
Expand Down Expand Up @@ -1004,7 +1015,6 @@ def first_level_from_bids(dataset_path,
supported_filters=[*_bids_entities()["raw"],
*_bids_entities()["derivatives"]],
extra_filter=img_filters,
verbose=verbose
)
StartTime = _infer_slice_timing_start_time_from_dataset(
bids_path=derivatives_path,
Expand Down Expand Up @@ -1061,16 +1071,14 @@ def first_level_from_bids(dataset_path,
sub_label=sub_label_,
task_label=task_label,
space_label=space_label,
img_filters=img_filters,
verbose=verbose)
img_filters=img_filters)
models_run_imgs.append(imgs)

events = _get_events_files(dataset_path=dataset_path,
sub_label=sub_label_,
task_label=task_label,
img_filters=img_filters,
imgs=imgs,
verbose=verbose)
imgs=imgs)
events = [pd.read_csv(event, sep='\t', index_col=None)
for event in events]
models_events.append(events)
Expand All @@ -1079,8 +1087,7 @@ def first_level_from_bids(dataset_path,
sub_label=sub_label_,
task_label=task_label,
img_filters=img_filters,
imgs=imgs,
verbose=verbose)
imgs=imgs)
if confounds:
confounds = [pd.read_csv(c, sep='\t', index_col=None)
for c in confounds]
Expand Down Expand Up @@ -1133,7 +1140,7 @@ def _list_valid_subjects(derivatives_path,
return set(sub_labels_exist)


def _report_found_files(
def _format_report_found_files(
files, text, sub_label, filters
):
"""Print list of files found for a given subject and filter.
Expand All @@ -1154,12 +1161,12 @@ def _report_found_files(
Only one filter per field allowed.

"""
print(
Copy link
Member

Choose a reason for hiding this comment

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

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

f"Found the following {len(files)} {text} files\n",
f"for subject {sub_label}\n",
f"for filter: {filters}:\n",
f"{files}\n",
)
return (f"\nFound the following {len(files)} {text} files\n" +
f"for subject {sub_label}\n" +
f"for filter: {filters}:" +
"\n- " +
"\n- ".join(files))



def _get_processed_imgs(
Expand All @@ -1168,7 +1175,6 @@ def _get_processed_imgs(
task_label,
space_label,
img_filters,
verbose
) :
"""Get images for a given subject, task and filters.

Expand All @@ -1189,9 +1195,6 @@ def _get_processed_imgs(
Filters are of the form (field, label).
Only one filter per field allowed.

verbose : :obj:`integer`
Indicate the level of verbosity.

Returns
-------
imgs : :obj:`list` of :obj:`str`
Expand All @@ -1204,7 +1207,6 @@ def _get_processed_imgs(
supported_filters=_bids_entities()["raw"]
+ _bids_entities()["derivatives"],
extra_filter=img_filters,
verbose=verbose,
)
imgs = get_bids_files(
main_path=derivatives_path,
Expand All @@ -1214,11 +1216,10 @@ def _get_processed_imgs(
sub_label=sub_label,
filters=filters,
)
if verbose:
_report_found_files(files=imgs,
text='preprocessed BOLD',
sub_label=sub_label,
filters=filters)
logger.info(_format_report_found_files(files=imgs,
text='preprocessed BOLD',
sub_label=sub_label,
filters=filters))
_check_bids_image_list(imgs, sub_label, filters)
return imgs

Expand All @@ -1229,7 +1230,6 @@ def _get_events_files(
task_label,
img_filters,
imgs,
verbose,
):
"""Get events.tsv files for a given subject, task and filters.

Expand All @@ -1254,9 +1254,6 @@ def _get_events_files(
imgs : :obj:`list` of :obj:`str`
List of fullpath to the preprocessed images

verbose : :obj:`integer`
Indicate the level of verbosity.

Returns
-------
events : :obj:`list` of :obj:`str`
Expand All @@ -1266,7 +1263,6 @@ def _get_events_files(
task_label=task_label,
supported_filters=_bids_entities()["raw"],
extra_filter=img_filters,
verbose=verbose,
)
events = get_bids_files(
dataset_path,
Expand All @@ -1276,19 +1272,17 @@ def _get_events_files(
sub_label=sub_label,
filters=events_filters,
)
if verbose:
_report_found_files(files=events,
text='events',
sub_label=sub_label,
filters=events_filters)
logger.info(_format_report_found_files(files=events,
text='events',
sub_label=sub_label,
filters=events_filters))
_check_bids_events_list(
events=events,
imgs=imgs,
sub_label=sub_label,
task_label=task_label,
dataset_path=dataset_path,
events_filters=events_filters,
verbose=verbose,
)
return events

Expand All @@ -1299,7 +1293,6 @@ def _get_confounds(
task_label,
img_filters,
imgs,
verbose,
):
"""Get confounds.tsv files for a given subject, task and filters.

Expand All @@ -1324,9 +1317,6 @@ def _get_confounds(
imgs : :obj:`list` of :obj:`str`
List of fullpath to the preprocessed images

verbose : :obj:`integer`
Indicate the level of verbosity.

Returns
-------
confounds : :obj:`list` of :obj:`str` or None
Expand All @@ -1344,7 +1334,6 @@ def _get_confounds(
task_label=task_label,
supported_filters=supported_filters,
extra_filter=img_filters,
verbose=verbose,
)
confounds = get_bids_files(
derivatives_path,
Expand All @@ -1354,11 +1343,10 @@ def _get_confounds(
sub_label=sub_label,
filters=filters,
)
if verbose:
_report_found_files(files=confounds,
text='confounds',
sub_label=sub_label,
filters=filters)
logger.info(_format_report_found_files(files=confounds,
text='confounds',
sub_label=sub_label,
filters=filters))
_check_confounds_list(confounds=confounds, imgs=imgs)
return confounds or None

Expand Down Expand Up @@ -1488,8 +1476,7 @@ def _make_bids_files_filter(
task_label,
space_label=None,
supported_filters=None,
extra_filter=None,
verbose=0
extra_filter=None
) :
"""Return a filter to specific files from a BIDS dataset.

Expand All @@ -1509,9 +1496,6 @@ def _make_bids_files_filter(
Filters are of the form (field, label).
Only one filter per field allowed.

verbose : :obj:`integer`
Indicate the level of verbosity.

Returns
-------
Filter to be used by :func:`get_bids_files`: \
Expand All @@ -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(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should warning be handled by the logger as well?

Copy link
Member

Choose a reason for hiding this comment

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

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

f"The filter {filter_} will be skipped. "
f"'{filter_[0]}' is not among the supported filters. "
f"Allowed filters include: {supported_filters}"
Expand Down Expand Up @@ -1625,7 +1608,6 @@ def _check_bids_events_list(
task_label,
dataset_path,
events_filters,
verbose
):
"""Check input BIDS events.

Expand Down Expand Up @@ -1689,7 +1671,6 @@ def _check_bids_events_list(
space_label=None,
supported_filters=supported_filters,
extra_filter=extra_filter,
verbose=verbose
)
this_event = get_bids_files(
dataset_path,
Expand Down
2 changes: 1 addition & 1 deletion nilearn/glm/tests/test_first_level.py
View file
Open in desktop
Expand Up @@ -1173,7 +1173,7 @@ def test_first_level_from_bids_select_all_runs_of_one_session(bids_dataset):
assert len(m_imgs[0]) == n_imgs_expected


@pytest.mark.parametrize("verbose", [0, 1])
@pytest.mark.parametrize("verbose", [0, 1, 2, 3])
def test_first_level_from_bids_smoke_test_for_verbose_argument(
bids_dataset,
verbose):
Expand Down