[MRG+1] Option to suppress validation for finiteness #7548

jnothman · 2016-10-02T02:52:24Z

Fixes #1363. I'm not sure if this is the best way to make a global configuration parameter available, or the best place to document it.

jnothman · 2016-10-02T03:06:20Z

And perhaps in the future we will add more validation suppression as a result of this flag (e.g. some of the stuff in utils.multiclass) and so we might need better ways to warn users that the behaviour of this flag may become more expansive without backwards compatibility concerns.

Is it worth using this option, for instance, to assume labels are already encoded in 0 to n_classes - 1? (or because this is a fit-time concern, is that premature optimisation?)

amueller · 2016-10-03T13:22:14Z

Currently the variable name is a bit to generic for my taste. It sounds like it suppresses all validation. Maybe FAST_VALIDATION?

jnothman · 2016-10-04T11:27:44Z

SUPPRESS_COSTLY_VALIDATION?? :) Problem with FAST_VALIDATION is the "only do it with care" factor. How about LESS_VALIDATION? PRESUME_VALID?

jnothman · 2016-10-05T01:10:36Z

I've renamed to PRESUME_FINITE

amueller · 2016-10-05T19:14:50Z

Sounds good. To me ASSUME_FINITE sounds more natural but you're the native speaker ;)

amueller · 2016-10-05T19:15:52Z

LGTM. I'd like to have @GaelVaroquaux sign off on this if he has time.

amueller · 2016-10-05T19:16:22Z

The documentation is quite hard to find right now, but I'm not sure what a better place would be.

jnothman · 2016-10-05T21:13:48Z

I'm happy to change to ASSUME. The difference is that often you assume something but still check; when you presume something, you go with that presumption blindly.

No hurry here.

NelleV · 2016-10-06T16:40:50Z

I like PRESUME_FINITE.
It looks good to me as well.

jnothman · 2016-10-07T03:33:47Z

But we do have assume_* elsewhere in the code (assume_centered and assume_unique) with the same meaning so perhaps we should follow that convention.

TomDLT · 2016-10-13T15:02:29Z

Small concern about putting the check inside assert_all_finite(X), as it is a public function advertised in the doc.
Shouldn't we just add it inside _ensure_sparse_format, check_array and check_X_y?

amueller · 2016-10-14T17:04:00Z

@TomDLT I'm not sure I understand your concern. We should document that the behavior is overwritten by the flag, but it doesn't change any current behavior.

jnothman · 2016-10-15T09:43:10Z

I think the idea that we're assuming rather than asserting finiteness when
the flag is on is pretty clear.

On 15 October 2016 at 04:04, Andreas Mueller notifications@github.com
wrote:

@TomDLT https://github.com/TomDLT I'm not sure I understand your
concern. We should document that the behavior is overwritten by the flag,
but it doesn't change any current behavior.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7548 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz6xVXXhOe7etRA3SvLIboMAiKrSQAks5qz7YCgaJpZM4KL6yC
.

jnothman · 2016-10-15T09:45:04Z

have added a note to docs.

NelleV · 2016-10-31T15:36:14Z

Are we still waiting for @GaelVaroquaux to comment on that one?

GaelVaroquaux · 2016-10-31T19:01:10Z

LGTM.

One think that would be good would be to add a contextmanager to do that. I fear Ill-written code that suppresses validation in more places that it would like.

jnothman · 2016-11-01T03:53:55Z

Do you foresee with sklearn.set_validation analogous to np.seterr? If we can agree on where it lives and how broadly it is named, I'm happy to implement here before merge. Otherwise, I think we could merge this PR as is (modulo what's new).

GaelVaroquaux · 2016-11-01T10:10:15Z

Do you foresee with sklearn.set_validation analogous to np.seterr?

Yes, I think that would be the right way to do it.

amueller · 2016-11-17T17:06:23Z

I'm fine with either way, slightly leaning to set_validation

jnothman · 2016-11-18T02:08:32Z

Yes, I'll get back to it some time soon! I know, it'll be a nice feature to have.

jnothman · 2016-12-20T13:32:46Z

@amueller, my concern with that first example is that introducing a context manger into func where there was none before wrecks everything from my_callback, despite some_dict not setting any of the same parameters as my_callback. Functions that use context managers then need to be documented for their imperviousness anyway.

I get the documentation simplicity of your approach. It makes sense in numpy where the errstate is homogeneous in the types of settings it's dealing with; and perhaps in matplotlib where the config is everything. Here we only have one setting, and as far as I can tell, configurations are unlikely to have much to do with each other. A context, except at the outermost level of library invocation, is likely to set only few of the params.

But this is almost entirely philosophy and I should probably just adopt what you suggest.

tacaswell · 2016-12-20T15:51:38Z

You currently only have one global configuration 😈. If you expect it to stay that way, then why aren't these called ignore_nan_context and set_ignore_nan_state, etc?

Users are very clever and will do all sorts of 'interesting' things (and will not be persuaded by 'do not do that', and documenting what not to do just gives some users ideas!) so even if sklearn never writes functions like this, some downstream library will.

jnothman · 2016-12-20T20:43:30Z

Shrug. I'll do it your way. Not entirely convinced it has real benefits.

…

On 21 December 2016 at 02:51, Thomas A Caswell ***@***.***> wrote: You *currently* only have one global configuration 😈. If you expect it to stay that way, then why aren't these called ignore_nan_context and set_ignore_nan_state, etc? Users are very clever and *will* do all sorts of 'interesting' things (and will not be persuaded by 'do not do that', and *documenting* what not to do just gives some users ideas!) so even if sklearn never writes functions like this, some downstream library will. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7548 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz68n7ACxZcLzLNodr2wZTA0TSFJFhks5rJ_mMgaJpZM4KL6yC> .

tacaswell · 2016-12-21T03:33:30Z

Sorry if I was arguing too forcefully or was out of line. I do not know the audience here (but do know I am the peanut gallery!).

jnothman · 2016-12-21T04:11:05Z

Arguing too forcefully probably doesn't exist for me ;)

…

On 21 December 2016 at 14:33, Thomas A Caswell ***@***.***> wrote: Sorry if I was arguing too forcefully or was out of line. I do not know the audience here (but do know I am the peanut gallery!). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7548 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_p2bsgDGrePqvmtpdNPEtJhNEkNks5rKJ4MgaJpZM4KL6yC> .

jnothman · 2016-12-21T10:32:32Z

Pushed conformant changes

jnothman · 2017-02-03T03:36:32Z

@amueller, I assume this still has your +1? Another review?

amueller · 2017-06-07T13:28:39Z

I'll review again and ping @GaelVaroquaux to check it out

amueller · 2017-06-07T15:05:24Z

sklearn/__init__.py

+import os
+from contextlib import contextmanager as _contextmanager
+
+_ASSUME_FINITE = bool(os.environ.get('SKLEARN_ASSUME_FINITE', False))


If we want to add more config in the future, don't we want them to be bundled in a dictionary?

amueller · 2017-06-07T15:07:10Z

still +1 but maybe have a the global variable be a dict if we want to add an option for the repr?

agramfort · 2017-06-07T16:08:25Z

sklearn/__init__.py

+
+    Parameters
+    ----------
+    assume_finite : bool, optional


bad param docstring

How so? Because I should say that it needs to be specified by name?

The point here is that the user always has the option to change, but can leave things unchanged Too

jnothman · 2017-06-07T21:23:16Z

The global variables is private so its structure can change

…

On 8 Jun 2017 1:07 am, "Andreas Mueller" ***@***.***> wrote: still +1 but maybe have a the global variable be a dict if we want to add an option for the repr? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7548 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6whM1-OzgvYU7Tbg1HUHb8hkqgjXks5sBryggaJpZM4KL6yC> .

amueller · 2017-06-08T11:22:41Z

Ok I'll do that in my PR ;)

jnothman · 2017-06-08T11:31:48Z

does that mean this one will get merged? :) can i still count nelle's approval above?

…

On 8 Jun 2017 9:22 pm, "Andreas Mueller" ***@***.***> wrote: Ok I'll do that in my PR ;) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7548 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz63mKh6xNJ8WJBCbHrS6v2sGcusZ5ks5sB9mCgaJpZM4KL6yC> .

agramfort · 2017-06-08T11:39:06Z

looks great.

@amueller green button ?

amueller · 2017-06-08T11:42:26Z

yes ;)

* ENH add suppress validation option * TST skip problematic doctest * Rename SUPPRESS_VALIDATION to PRESUME_FINITE * Change PRESUME_ to ASSUME_ for convention's sake * DOC add note regarding assert_all_finite * ENH add set_config context manager for ASSUME_FINITE * Make ASSUME_FINITE private and provide get_config * Fix ImportError due to incomplete change in last commit * TST/DOC tests and more cautious documentation for set_config * DOC what's new entry for validation suppression * context manager is now config_context; set_config affects global config * Rename missed set_config to config_context * Fix mis-named test * Mention set_config in narrative docs * More explicit about limmited restoration of context * Handle case where error raised in config_context * Reset all settings after exiting context manager

ENH add suppress validation option

1cbf18b

jnothman force-pushed the suppress-validation branch from 12dd724 to 1cbf18b Compare October 2, 2016 02:52

TST skip problematic doctest

f0878d0

Rename SUPPRESS_VALIDATION to PRESUME_FINITE

4d35dd8

amueller changed the title ~~[MRG] Option to suppress validation for finiteness~~ [MRG + 1] Option to suppress validation for finiteness Oct 5, 2016

NelleV approved these changes Oct 7, 2016

View reviewed changes

NelleV changed the title ~~[MRG + 1] Option to suppress validation for finiteness~~ [MRG + 2] Option to suppress validation for finiteness Oct 7, 2016

Change PRESUME_ to ASSUME_ for convention's sake

2530a47

DOC add note regarding assert_all_finite

379302c

jnothman mentioned this pull request Nov 1, 2016

[MRG] Added flag to disable l2-dist finite check #7383

Open

Reset all settings after exiting context manager

089339c

jnothman mentioned this pull request Jun 7, 2017

[RFC] Simple __repr__ with global flag #9039

Closed

Merge branch 'master' into suppress-validation

b91c66e

amueller reviewed Jun 7, 2017

View reviewed changes

agramfort reviewed Jun 7, 2017

View reviewed changes

amueller merged commit ee88cf4 into scikit-learn:master Jun 8, 2017

jondo mentioned this pull request Sep 10, 2017

[MRG] SVM: Ensure nonnegative sample weights (#9494) #9674

Closed

rth mentioned this pull request Jan 8, 2018

[MRG+1] ENH Add working_memory global config for chunked operations #10280

Merged

10 tasks

[MRG+1] Option to suppress validation for finiteness #7548

[MRG+1] Option to suppress validation for finiteness #7548

Conversation

jnothman commented Oct 2, 2016

jnothman commented Oct 2, 2016 • edited Loading

amueller commented Oct 3, 2016

jnothman commented Oct 4, 2016

jnothman commented Oct 5, 2016

amueller commented Oct 5, 2016

amueller commented Oct 5, 2016

amueller commented Oct 5, 2016

jnothman commented Oct 5, 2016

NelleV commented Oct 6, 2016

jnothman commented Oct 7, 2016 • edited Loading

TomDLT commented Oct 13, 2016

amueller commented Oct 14, 2016

jnothman commented Oct 15, 2016

jnothman commented Oct 15, 2016

NelleV commented Oct 31, 2016

GaelVaroquaux commented Oct 31, 2016

jnothman commented Nov 1, 2016

GaelVaroquaux commented Nov 1, 2016 via email

amueller commented Nov 17, 2016

jnothman commented Nov 18, 2016

jnothman commented Dec 20, 2016

tacaswell commented Dec 20, 2016

jnothman commented Dec 20, 2016 via email

tacaswell commented Dec 21, 2016

jnothman commented Dec 21, 2016 via email

jnothman commented Dec 21, 2016

jnothman commented Feb 3, 2017

amueller commented Jun 7, 2017

amueller Jun 7, 2017

Choose a reason for hiding this comment

amueller commented Jun 7, 2017

agramfort Jun 7, 2017

Choose a reason for hiding this comment

jnothman Jun 7, 2017

Choose a reason for hiding this comment

agramfort Jun 8, 2017

Choose a reason for hiding this comment

jnothman commented Jun 7, 2017 via email

amueller commented Jun 8, 2017

jnothman commented Jun 8, 2017 via email

agramfort commented Jun 8, 2017

amueller commented Jun 8, 2017

jnothman commented Oct 2, 2016 •

edited

Loading

jnothman commented Oct 7, 2016 •

edited

Loading