throw warning and update error message

[CloseChoice]

Overview

Supports #3574

Description of the changes proposed in this pull request:

• throw warning again. I checked the commit of Scott, where he uncommented the warning (see here) but could not find any reason why he did, so no linked PR or something else.
• update error message in case of uncovered leaves

Checklist

• All pre-commit checks pass.
• Unit tests added (if fixing a bug or adding a new feature)

[CloseChoice]

@DanGolding: happy if you also take a look

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.46%. Comparing base (74db96c) to head (4fb3d8a).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3576   +/-   ##
=======================================
  Coverage   61.46%   61.46%           
=======================================
  Files          90       90           
  Lines       12746    12747    +1     
=======================================
+ Hits         7834     7835    +1     
  Misses       4912     4912           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[connortann]

This change may impact a large fraction of users, so I think we should carefully consider what level of notification is appropriate. Would logging.info be more appropriate here?

In the logging HOWTO guide, they recommend:

• To report events that occur "during normal operation of a program", use logging info()
• To issue a warning regarding a particular runtime event, if the issue is avoidable and the client application should be modified to eliminate the warning, use warnings.warn

To me, this seems like normal operation rather than an "issue", as this behaviour is described in the docs of tree explainer:

feature_perturbation : "interventional" (default) or "tree_path_dependent" (default when data=None)

My guess is that Scott commented this warning as it was becoming a bit annoying.

[connortann]

One suggestion as above

[CloseChoice]

Your suggestion is implemented. Sorry that this took so long

[connortann]

I believe logging.warn is deprecated in favour of logging.warning.

Also, we should use a module-level logger rather than the root logger: log = logging.getLogger(__name__).

From the python howto docs:

It is strongly advised that you do not log to the root logger in your library. Instead, use a logger with a unique and easily identifiable name, such as the name for your library’s top-level package or module. Logging to the root logger will make it difficult or impossible for the application developer to configure the logging verbosity or handlers of your library as they wish.

[CloseChoice]

I believe logging.warn is deprecated in favour of logging.warning.

Also, we should use a module-level logger rather than the root logger: log = logging.getLogger(__name__).

From the python howto docs:

It is strongly advised that you do not log to the root logger in your library. Instead, use a logger with a unique and easily identifiable name, such as the name for your library’s top-level package or module. Logging to the root logger will make it difficult or impossible for the application developer to configure the logging verbosity or handlers of your library as they wish.

Thanks for the review. I implemented your suggestion to have a custom logger for the _tree.py file

[connortann]

LGTM!

[connortann]

Ah - probably the same discussion applies here about logging.INFO vs logging.WARNING. I'll hold off merging, pending the dicussion in our other thread...

#3650 (comment)