Skip to content

[WIP] Prototype 1 for strict check_estimator mode #16882

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

Closed
wants to merge 3 commits into from

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Apr 9, 2020

Closes #14929
Fixes #13969
Fixes #16241

See also alternative #16890 and #17252

Probably too hacky but why not...

@NicolasHug
Copy link
Member Author

OK, test failure is normal (it's because we print in a test, which is going to be removed of course).

@rth @jnothman @thomasjpfan WDYT, are you OK with this idea of marking strict checks with a decorator? I'm not a fan of the use of the global var, interested in your input

Thanks

@rth
Copy link
Member

rth commented Apr 9, 2020

Thanks for giving this a go @NicolasHug !

If we want to avoid global variable, I guess the other solution is to mark the checks to skip in a decorator in some way (e.g. set an attribute),

def strict_check(check):
    setattr(check, 'strict', True)
    # do we actually need to use the wrapper in this case?
    # the signature is preserved as far as I can tell
    return check

then pass strict_mode to _generate_class_checks and _generate_instance_checks, and filter/raise warnings there. Currently I feel that's a bit strange that an individual check is run or not based on a global variable even when run from outside of check_estimator.

Otherwise yes, why not for decorators.

@rth
Copy link
Member

rth commented Apr 9, 2020

Although the other use case this doesn't address is to run the test but skip /modify some of it's parts when strict=False. That would require passing strict variable to the check.

If we do that, calling some function skip_if_strict(strict) inside is only marginally longer than decorators. Though it would mean skipped tests still show in pytest reports (e.g. with parametrized_checks) so maybe decorators as a separate mechanism are still useful.

@NicolasHug
Copy link
Member Author

Although the other use case this doesn't address is to run the test but skip /modify some of it's parts when strict=False

Yeah if we want to allow this I think we need to pass a strict parameter to all checks. I'll try to submit another PR with that workflow enabled

@NicolasHug
Copy link
Member Author

(Thanks for the feedback!)

@NicolasHug NicolasHug changed the title [WIP] Prototype for strict check_estimator mode [WIP] Prototype 1 for strict check_estimator mode Apr 10, 2020
@rth rth closed this in #17361 Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants