Add _check_option function #5958

wmvanvliet · 2019-02-19T13:32:01Z

As @massich noted in #5946, the following snippet is a common occurrence in our codebase:

# Validate method parameter
if method not in ['foo', 'bar', 'baz']:
    raise ValueError("Invalid value for 'method' parameter. Valid options are ...")

Generating a good error message takes a bit more work. You can either do:

valid_options = ['foo', 'bar', 'baz']
if method not in valid_options:
    raise ValueError("Invalid value for 'method' parameter. Valid options are %s, "
                     "but got '%s' instead." % (valid_options, method))

or go just write it all out if you don't like the list-like formatting of the valid options:

if method not in ['foo', 'bar', 'baz']:
    raise ValueError("Invalid value for 'method' parameter. Valid options are "
                     "'foo', 'bar' and 'baz', but got '%s' instead." % method)

It's a little bit of a hassle and we're not very consistent across the code base in how we generate such errors.

This PR adds a private _check_option function that you can use to perform such a check and generate a friendly error message in a single line:

_check_option('method', method, valid_options=['foo', 'bar', 'baz'])

Small QOL improvement or unnecessary bloat? You be the judge :)
If we agree that we want this function, I'll open a follow-up PR that changes the bulk of the occurrences of this pattern in our codebase.

larsoner · 2019-02-19T14:42:11Z

+1 from me

agramfort · 2019-02-19T15:07:18Z

mne/utils/check.py

+
+
+def _check_option(parameter, value, allowed_values, error=ValueError):
+    """Checks a the value of a parameter against a list of valid options.


a the -> the

agramfort · 2019-02-19T15:08:29Z

can you use/integrate this code anywhere? Let's clean at the same time.

massich · 2019-02-19T15:14:48Z

+1000

I found myself refactoring again here https://github.com/mne-tools/mne-python/pull/5836/files#diff-d8a578281e14fb43fe3a386a41036b8fR794

So if you had not opened this PR I would have done it.

I would put the word rise in the function name ~~and maybe we could add the type of error to raise~~(this is already there). But if you have a bulk of examples, maybe we can just start by this and then check if we can refactor further.

massich · 2019-02-19T15:22:17Z

mne/utils/tests/test_check.py

    assert list(range(7, 10)) == picks
+
+
+def test_check_option():


This is a good usecase to use pytest.mark.parametrize

Thanks for pointing me to this function. However, using pytest.mark.parametrize ended up not being any shorter nor clearer than these four lines. A little too much changes across each line.

...or I just don't understand the parametrize function too well. If you have some clever refactoring in mind, please share :)

wmvanvliet · 2019-02-19T17:34:22Z

I would put the word rise in the function name

(I think you meant raise)

I totally see how that is good practice. But in this case, all the functions in the utils.check module start with _check_ (and many raise errors). Hence I feel that in this particular case, consistency beats this otherwise excellent guideline.

wmvanvliet · 2019-02-19T17:35:51Z

can you use/integrate this code anywhere? Let's clean at the same time.

I'll put in a few use cases in this PR. But shall we leave be big cleanup for a separate PR that will strictly be a search and replace?

agramfort · 2019-02-19T20:22:38Z

git grep "Got "

should help find some places you can use this new check function

I would favor to do the clean up in this PR. But others can help by pushing to the PR directly.

Just saying :)

wmvanvliet · 2019-02-20T10:43:33Z

I would favor to do the clean up in this PR.

Then you got it!

agramfort · 2019-02-20T16:56:07Z

then I go for it or you go for it?

wmvanvliet · 2019-02-20T16:57:40Z

Lets both go for it! There are plenty of cases to cover I guess :) And I'm still on paternity leave so my working time is spotty.

codecov · 2019-02-20T17:05:48Z

Codecov Report

Merging #5958 into master will increase coverage by 0.08%.
The diff coverage is 99.31%.

@@            Coverage Diff             @@
##           master    #5958      +/-   ##
==========================================
+ Coverage   88.83%   88.92%   +0.08%     
==========================================
  Files         401      401              
  Lines       72874    72810      -64     
  Branches    12179    12108      -71     
==========================================
+ Hits        64741    64745       +4     
+ Misses       5215     5180      -35     
+ Partials     2918     2885      -33

massich · 2019-02-20T17:29:50Z

Looking at the API I would prefer something like this:

def _check_option(*, option, accepted_values, error):
       raise ...

my_var = None
_check_option(option=my_var, accepted_values=[Ture, False], error=ValueError)

and that it prints "valid values for my_var are "True" or "False"; got None"
I comparing it to _check_option('my_var', my_var, [Ture, False]) I find that if someone knows nothing about the whatever does _check_option reads the first one it is clear what it does (and that it will raise an error).

(We can allow for not passing option name _check_option(my_var, accepted_values=[Ture, False], error=ValueError). Also I'm not suggesting that we should put * to force it. This is just to illustrate what I'm refering to and then the rest be a convetion. We can even inforce such convention with LGTM)

If you don't find it useful, we can let it for someone who wants to drill in a rainy day. (here's a stack overflow)

larsoner · 2019-02-20T17:42:37Z

I prefer it the way it is currently. It's really an expert/dev option and I don't find all the extra chars in our repo will be helpful in the long run. Extra verbosity can hurt readability sometimes, it's a trade-off. Here I think the first is better for something that is meant to be a time-saver anyway.

mne/utils/check.py

massich · 2019-02-20T17:46:07Z

mne/utils/check.py

+    Returns
+    -------
+    is_valid : bool
+        Whether the value was valid or not.


if its not valid, you are not returning. I'm not sure if we should say it. or just say return True

Good point.

I would remove the "Returns" section as it returns None or raises a ValueError.

besides LGTM

@wmvanvliet this last one please

agramfort · 2019-02-20T20:07:42Z

API of private function is fine with me as is. Let's use it as much as possible everywhere now.

wmvanvliet · 2019-02-20T22:03:05Z

That's filenames stating with A-E done. Lots more to do.

mne/channels/layout.py

larsoner · 2019-02-24T10:08:17Z

This pull request introduces 1 alert when merging 692fca1 into 5418605 - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

Comment posted by LGTM.com

massich · 2019-02-24T19:17:33Z

mne/utils/check.py

+        options = 'Allowed values are '
+        options += ', '.join(['%r' % v for v in allowed_values[:-1]])
+        options += ' and %r' % allowed_values[-1]
+    raise ValueError(msg.format(parameter=parameter, options=options,


are you sure that it will always be a ValueError? I'm fine with the change and change it back if a TypeError appears.

Scratch that. Actually to check if an option is a subclass of the allowed_values the function should be quite different:

def _check_option(option_name, option_val, allowed_values, type_of_check='one of', ...):

and allow for type_of_check='sublcass', etc..

Why would I want to check whether an option is a subclass of allowed_values?

I've gone through the entire codebase now. This function only has to deal with str, None, True and False. It also only ever needs to throw ValueError, hence I removed the code for throwing other types of errors.

The reason this function doesn't need to be more complicated than this, is that all it really does is form a very specific error message that just happens to be a very common one. When a different kind of check is required, the error message is likely to be different as well and you're better off not bending over just to use this function but just write the if-statement and throw yourself.

I agree. Let’s not be too smart here. @wmvanvliet can you just see why CIs complain ?

wmvanvliet · 2019-02-27T07:44:57Z

I think I'm done here. Ready for merge!

larsoner

LGTM, let's merge once the CIs come back happy

larsoner · 2019-02-27T14:23:38Z

Thanks @wmvanvliet !

massich · 2019-03-04T10:04:56Z

THX @wmvanvliet

* Add _check_option function * Start putting _check_option to use * More usage of _check_option * Fixes * Simplify _check_option function * More _check_option usage * fix * Last batch of _check_option usage * fix * Fixes * Update _check_option doc

agramfort reviewed Feb 19, 2019

View reviewed changes

massich reviewed Feb 19, 2019

View reviewed changes

massich mentioned this pull request Feb 19, 2019

FIX: check weight_norm parameter #5946

Merged

massich reviewed Feb 20, 2019

View reviewed changes

mne/utils/check.py Outdated Show resolved Hide resolved

massich reviewed Feb 20, 2019

View reviewed changes

agramfort reviewed Feb 21, 2019

View reviewed changes

mne/channels/layout.py Outdated Show resolved Hide resolved

wmvanvliet added 2 commits February 24, 2019 09:29

Add _check_option function

fe18cac

Start putting _check_option to use

973289d

wmvanvliet force-pushed the check_option branch from b2e529d to 973289d Compare February 24, 2019 07:30

wmvanvliet added 4 commits February 24, 2019 10:17

More usage of _check_option

ff726e7

Fixes

0811f65

Simplify _check_option function

0bbbd31

More _check_option usage

692fca1

wmvanvliet added 2 commits February 24, 2019 13:22

fix

dbb1ddd

Last batch of _check_option usage

b045f4a

fix

53df40e

massich reviewed Feb 24, 2019

View reviewed changes

Fixes

4b3c444

Update _check_option doc

85eaacf

larsoner approved these changes Feb 27, 2019

View reviewed changes

larsoner merged commit 73ea718 into mne-tools:master Feb 27, 2019

wmvanvliet deleted the check_option branch March 4, 2019 12:29



		def _check_option(parameter, value, allowed_values, error=ValueError):
		"""Checks a the value of a parameter against a list of valid options.

Uh oh!

Add _check_option function #5958

Add _check_option function #5958

Uh oh!

Conversation

wmvanvliet commented Feb 19, 2019

Uh oh!

larsoner commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Feb 19, 2019

Uh oh!

massich commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmvanvliet commented Feb 19, 2019

Uh oh!

wmvanvliet commented Feb 19, 2019

Uh oh!

agramfort commented Feb 19, 2019

Uh oh!

wmvanvliet commented Feb 20, 2019

Uh oh!

agramfort commented Feb 20, 2019 via email

Uh oh!

wmvanvliet commented Feb 20, 2019

Uh oh!

codecov bot commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

massich commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Feb 20, 2019

Uh oh!

Uh oh!

massich Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Feb 20, 2019

Uh oh!

wmvanvliet commented Feb 20, 2019

Uh oh!

Uh oh!

larsoner commented Feb 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmvanvliet Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmvanvliet commented Feb 27, 2019

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Feb 27, 2019

Uh oh!

massich commented Mar 4, 2019

Uh oh!

massich commented Feb 19, 2019 •

edited

Loading

codecov bot commented Feb 20, 2019 •

edited

Loading

massich commented Feb 20, 2019 •

edited

Loading

massich Feb 20, 2019 •

edited

Loading

wmvanvliet Feb 25, 2019 •

edited

Loading