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

Use the MNE logger to set the verbosity #105

Closed
mscheltienne opened this issue Oct 14, 2021 · 6 comments · Fixed by #112
Closed

Use the MNE logger to set the verbosity #105

mscheltienne opened this issue Oct 14, 2021 · 6 comments · Fixed by #112
Milestone

Comments

@mscheltienne
Copy link
Contributor

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
Copy link
Owner

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 🤔 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
Copy link
Contributor Author

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

@sappelhoff sappelhoff added this to the 0.5.0 milestone Oct 14, 2021
@mscheltienne mscheltienne changed the title Adding a verbose argument/setting to limit the prints Use the MNE logger to set the verbosity Oct 14, 2021
@a-hurst
Copy link
Collaborator

a-hurst commented Oct 21, 2021

@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
Copy link
Owner

ok 👍 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.

@sappelhoff sappelhoff mentioned this issue Oct 22, 2021
@mscheltienne
Copy link
Contributor Author

@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
Copy link
Owner

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.

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.

3 participants