sappelhoff / pyprep

A Python implementation of the Preprocessing Pipeline (PREP) for EEG data
https://pyprep.readthedocs.io/en/latest/
MIT License
128 stars 30 forks source link

Use the MNE logger to set the verbosity #105

Closed mscheltienne closed 2 years ago

mscheltienne commented 2 years ago

I have been using PyPREP a bit (version 0.3.1, pip install) and I really like it for now. Would it be possible to add a verbose argument to limit the prints? Alternatively, moving the print to a logger with a set_log_level function would work out too.

Example: print("Executing RANSAC\nThis may take a while, so be patient...")

When running a preprocessing pipeline with several steps, one of which is PyPREP, the relevant logging from the pipeline can be completely overwhelmed by the prints from PyPREP, especially if you run it as I do on e.g. 40 cores at once.

sappelhoff commented 2 years ago

I have been using PyPREP a bit (version 0.3.1, pip install) and I really like it for now.

oh, please install and use the development version for now:

pip install git+https://github.com/sappelhoff/pyprep.git@master

as we note in the README: https://github.com/sappelhoff/pyprep#installation

We are preparing for a bigger release, and the current development version fixes many inaccuracies and problems that are still there in the 0.3.1 release.

cc @a-hurst @yjmantilla should we maybe just release in the current state and do the remaining features in 0.5 or 0.4.1 or so? I am concerned that too many people will use a standard pip install and end up with version 0.3.1


Regarding your proposal @mscheltienne --> yes, I think we could switch to using the MNE logger :thinking: then you could easily set any log level you want. But it's gonna be a bit of work to consistently apply that throughout the code base.

mscheltienne commented 2 years ago

@sappelhoff Thank you for your concern, and as I was posting this issue for a logger/verbosity, I did read quickly through the other issues, and I have now upgraded to the latest development version. I'll rerun my pipeline on my data, it's just a couple of hours on our servers, not a big deal. I don't know how many users you have on PyPREP, but I doubt many of them read the issues on GitHub, or the readme, or even connect to the GitHub page. conda or pip install are way more common, and if you have major bugs/problems that have been fixed, I would suggest you do an intermediate release.

Logger from MNE looks like the way to go. Of course, this is not urgent, but keep it in mind for a future release 😉.

a-hurst commented 2 years ago

@sappelhoff I think releasing a version 0.3.5 or 0.4 in its current state is probably fine! I'm busy with the first semester of my PhD so I can't contribute too much right now, but our reading week is coming up in November and I can set aside some time to finally finish PR #99.

One thing to note is that upstream PREP has fixed one of the bugs we reported so matlab_strict and matprep_artifacts would need minor updates to match. I didn't tackle this earlier because Dr. Robbins said she was going to look at some of the other bugs in more depth, so I thought it'd make sense to wait until that happened, but given the lack of follow-up it's probably best to tackle that before a release.

sappelhoff commented 2 years ago

ok :+1: I'll just quietly release 0.4 now, so that people who download from pypi/conda-forge will end up with the good version. We can then make a bigger announcement and celebrating when we picked off the remaining points on the list.

mscheltienne commented 2 years ago

@sappelhoff For the logger, do you want to directly use the MNE one with from mne.utils import logger, or do you want to duplicate it into a similar logger in pyprep? If it's the first one, I can do it quickly for you if you want, finding all the prints and replacing them with logger messages won't take long.

sappelhoff commented 2 years ago

Hey @mscheltienne, given that MNE will always remain a core dependency of this package, I'd like to use the MNE logger for simplicity :-) A PR to add this would be very welcome.