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

change to log message and log level in feature scoring #318

Merged
merged 1 commit into from Dec 1, 2023

Conversation

quaquel
Copy link
Owner

@quaquel quaquel commented Nov 20, 2023

fixes #261

columns with the same value for all entries don't matter for feature scoring, so they can be ignored.

fixes #261

columns with the same value for all entries don't matter for feature scoring, so they can be ignored.
@quaquel quaquel added this to the 2.5.0 milestone Nov 20, 2023
@coveralls
Copy link

Coverage Status

coverage: 80.206%. remained the same
when pulling 1739302 on fs_logmessage
into b6ac5cc on master.

@EwoutH
Copy link
Collaborator

EwoutH commented Nov 21, 2023

But it's still nice to know that they are ignored, right? If so, maybe they shouldn't be hidden in a debug log.

@quaquel
Copy link
Owner Author

quaquel commented Nov 21, 2023

I was debating this with myself as well. What annoys me in the current implementation is that you can get multiple messages about columns that are removed (e.g., policy, model). Moreover, in other places, we remove those columns silently anyway.

@EwoutH
Copy link
Collaborator

EwoutH commented Nov 21, 2023

My instinct is to follow PEP 20 here:

Explicit is better than implicit.

So rather than removing it here, we might want to add it in other places that make sense.

That said, maybe some columns dropping are special cases: policy and scenario might be two of them. If you only have one policy (and test different scenarios / uncertainty), it might be logical that policy is dropped (so it's indented behaviour). In other cases, it might not.

So that's I think what my original issue was about: I got the message in #261, now what? It should either give something concrete to consider, or some call to action.

@quaquel
Copy link
Owner Author

quaquel commented Nov 23, 2023

It is not a call to action, hence my choice to lower the log-level. The original idea of the message was more a 'just so you know' type message. One way of solving the issue is to make this behavior clear in the documentation while also lowering the log level as I have now done.

@EwoutH
Copy link
Collaborator

EwoutH commented Nov 23, 2023

Good point, but a “just so you know” message is exactly where the info level if for, right? The warn level is for actions to consider, and the debug to have as much information available as possible to find unexpected behaviour.

Are there any special cases we can threat to make it more clear?

(on a higher level, I’m fine with all options, since this is really minor)

@quaquel quaquel merged commit 2002532 into master Dec 1, 2023
22 checks passed
@quaquel quaquel deleted the fs_logmessage branch December 1, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature_scoring: Improve drop column log entry
3 participants