choderalab / pymbar

Python implementation of the multistate Bennett acceptance ratio (MBAR)
http://pymbar.readthedocs.io
MIT License
238 stars 92 forks source link

Check proper verbosity of mbar #379

Open mrshirts opened 4 years ago

mrshirts commented 4 years ago

Doesn't appear to be printing out much anymore when verbose=True is set.

mrshirts commented 4 years ago

OK, the issue is that setting verbose sends the desired text to logger.info, but doesn't actually print it. Presumably, if one sets verbose, one wants that info to be printed out in some way, but right now, it's getting lost. @jaimergp , suggestions for sending that information to where the user can see it?

jaimergp commented 4 years ago

The logging module works on the assumption that emission of messages is decoupled of delivery. In this decoupling, libraries are only emitters, and it's up to the applications to configure how to deliver the message. So the user needs to configure logging to print stuff if they want to (usually, logging.basicConfig() is enough).

Since pymbar is a library, does it make sense for us to configure the verbosity? Do we have an CLI entry point? We can configure logging globally with some settings (e.g. with a singleton like pymbar.settings.verbose = True, but in principle we shouldn't.

jaimergp commented 4 years ago

That's why all these if verbose: checks do not make much sense in our case. We can (and should) leave all the logger.{info,debug,warn,error} calls unchecked and, if anything, configure the verbosity at a global level.

mrshirts commented 4 years ago

So what would your suggestion be to have the code perform how people would expect, that if they give a verbosity argument, it increases the amount of information out? What hierarchy of flags to use? Where and when to send the output out (right now, most of the output occurs near the data that is being generated).

jaimergp commented 4 years ago

Ideally, there would be no verbose flag at the function/class level. Users are expected to set the logging level in their scripts if desired.

import logging
logging.basicConfig(level=logging.DEBUG)

# proceed with your pymbar calling code

To transition from the current approach, last version of pymbar 3 should include a deprecation warning for verbose=True:

>>> some_result = some_function(verbose=True)
DeprecationWarning("Pymbar v4.0+ will use `logging` for debug, informative reports and warnings. To increase output verbosity in the future, please use `logging.basicConfig(level=logging.<DESIREDLEVEL>)` in your scripts")

IIRC, there's a context manager you can use for temporary verbosity increase. The main idea is that emission/delivery are uncoupled in terms of responsibility, but it still happens synchronously if you want to. There's more info here (check the "Using handlers" section).

mrshirts commented 4 years ago

I'm OK with that. However, note that a lot of people (including me) are operating in the "UNIX command" mentality, where you trigger a flag to increase verbosity. Adding functionality doesn't always improve user experience as one would like if people are not used to using the functionality.

So it will have to really be well documented that one does this, and the default level should probably be verbose on, with maybe a command itself telling people how to adjust the verbosity (i.e., we set the default level, and tell them how to change it).

jaimergp commented 4 years ago

Yes, I totally agree. Those "app-level" flags can still be set up internally with the context manager if necessary. That's why I have been added all those "ideally". I understand it's a big change for the end user who just wants some stuff printed to stdout.

I'll try to come up with a design that addresses all of our concerns. Now that we have logging, all the configuration possibilities are there. We just need to agree on the best default config for our use case (which should be as close to the pymbar3 behavior as possible).

mrshirts commented 4 years ago

Now that the PMF reformatting is in, we are getting closer to getting something that can be pymbar 4.0. @jaimergp, do you have a suggested design here for the context manager in terms of app-level flags to print expected output from the logger?

jaimergp commented 4 years ago

I have been thinking about this, and we might want to implement a package-level Settings instance that can configure the logging for all pymbar to a sensible default. Changing the value on the Settings instance will propagate that to the logger automatically.

With respect to the context manager for the verbose flag, we could provide something like this.

jaimergp commented 4 years ago

I can try to implement this next week, if that's ok?

mrshirts commented 4 years ago

I'll defer to you on what makes the most sense and is most maintainable going forward.

jaimergp commented 4 years ago

Check #391 !