Integrate Loguru #62
Integrate Loguru #62
Conversation
|
Regarding dependencies, in this PR I just added loguru to Then I saw #53 and found this proposed line: To install ``peakdet`` along with the basic required modules just run::
pip3 install phys2bidsWhy not have such dependencies in the requirements file to not require installing |
|
Hey @maestroque! Our main way to install things is through pip (specified and regulated by the When you want to update setups, you can add the required packages in the requirements.txt, but you have to add them in the correct category in the You do not need to install phys2bids. |
|
Currently the logs for errors are very verbose regarding callbacks. Do we want this or should we opt for something shorter? I attach an example below |
|
This seems extremely verbose... Should we give the bare minimum at any level, and then increase the level when a "verbose" CLI call is selected (see phys2bids)? |
|
It can toggled by adding a new logger instance with Another question is if someone just uses the modules on their own as a library instead of the CLI, should we assume that the user then is responsible for their own logger (check here)? Or do we still use loguru? |
That seems sensible to me! Our logging is really meant mainly for the CLI, but consider that it will be a required install once we add it, so our module users should still be able to use it if they want to. |
|
The last issue is that
with logger.catch():
peakdet.utils.enable_logger(False, True)
data = np.random.rand(5000, 2)
phys = peakdet.Physio(data, 10)What's your opinion with this? Other than that I believe this PR is ready for review. |
|
Btw a sample log would be something like this with the and a sample log without |
|
Again, seems fair to me. @m-miedema @me-pic what do you think? |
|
Looks good to me too ! Good job 🎉 |
There was a problem hiding this comment.
@maestroque great start! There are a few points that we might want to discuss about regarding implementation. A few others are mainly style related, so no worries!
Looks good to me! |
|
One last change. In some test cases the test was checking if a --- with pytest.warns(UserWarning):
--- arr = io.load_physio(np.loadtxt(get_test_data_path('ECG.csv')))
+++ arr = io.load_physio(np.loadtxt(get_test_data_path("ECG.csv")))
+++ assert "WARNING" in caplog.textwhere also a It just tests that a warning message was sent, similarly to the previous version where it just checked for a user defined warning. The only thing that's different is that if multiple warnings are expected per unit test, this assertion is not enough. assert caplog.text.count("WARNING") == 1I believe it's a sufficient warning assertion. Let me know your thoughts @smoia @m-miedema @me-pic |
This seems like a reasonable solution to me, but @smoia has more experience with how the tests were originally set up here! |
There was a problem hiding this comment.
I'm wondering if we can discuss the logger enabling you added on Thursday, I'm finding it a bit tricky to understand how this will be intuitive to use without the CLI. This may just be a question of adding some documentation. Otherwise, everything looks good to go!
|
@m-miedema we can discuss it sure! You could also elaborate here if you want.
Do you find a specific part confusing? Let me know your thoughts. |
github-actions
bot
added
Internal
There was a problem hiding this comment.
This again seems ready to go except for some minor changes to the enable_logger function. If possible, allow this to be configured more than once. Otherwise, open another issue. The docstring for that function looks good for now, but I think eventually adding more external documentation for logging will be helpful both for external users working interactively with the library and for less experienced members of the community developing the packages. We can then possibly link to it in the docstring?
|
@m-miedema I added some extra functions to disable the logger and configure the log level. Then also the appropriate warnings and errors are added to point the users to the correct usage of the functions. |
Looks great, thank you for adding these! Just waiting to hear back from @smoia to proceed with the merge. |
| @@ -34,3 +36,5 @@ | |||
|
|
|||
| __version__ = get_versions()["version"] | |||
| del get_versions | |||
|
|
|||
| logger.disable("peakdet") | |||
There was a problem hiding this comment.
I'm sorry, I'm thick about this, but why are we importing and then disabling for peakdet?
There was a problem hiding this comment.
So that the loguru logger is disabled by default for library users, so they can enable it explicitly if they want to. This is not the case for the CLI however
peakdet/cli/run.py
Outdated
| edit_group.add_argument( | ||
| "-debug", | ||
| "--debug", | ||
| dest="debug", | ||
| action="store_true", | ||
| help="Only print debugging info to log file. Default is False.", | ||
| default=False, | ||
| ) | ||
| edit_group.add_argument( | ||
| "-quiet", | ||
| "--quiet", | ||
| dest="quiet", | ||
| action="store_true", | ||
| help="Only print warnings to log file. Default is False.", | ||
| default=False, | ||
| ) | ||
| edit_group.add_argument( | ||
| "-verbose", | ||
| "--verbose", | ||
| dest="verbose", | ||
| action="store_true", | ||
| help="Print verbose error logs with diagnostics", | ||
| default=False, | ||
| ) |
There was a problem hiding this comment.
Could you add these three into a mutually exclusive group of arguments?
There was a problem hiding this comment.
It should be good now
…otlib and click.edit import from cli/run.py
|
🚀 PR was released in |
References: physiopy/phys2bids#461
Integrate Loguru for all log types, including info messages, warnings and exception catching
Proposed Changes
print()s, warnings and exceptionsChange Type
bugfix(+0.0.1)minor(+0.1.0)major(+1.0.0)refactoring(no version update)test(no version update)infrastructure(no version update)documentation(no version update)otherChecklist before review