[ENH] Testing for erroneous use of reserved attributes

[fkiraly]

All base classes have some reserved attributes, see PR #2769, and we have already run into the problem twice, of concrete estimators using attributes that have the same name as reserved attributes.

We should test for this, but ... how?

Any good ideas, @miraep8? Given that you are the other person who ran into this...

[miraep8]

I can implement this!

Here is an idea - we have a class attribute which is a set of protected names in BaseEstimator. Other classes (ie BaseForecaster and BaseTransformer inherit from BaseEstimator and can add more protected names to.
this set of names.

Then we add a test for every concrete implementation of an estimator we make sure none of its class specific attributes are in the set of banned names? (ie we could exclude any Base classes (or HeterogenousMetaEstimator) from the tests). The thought here is that these protected attributes should only be added in abstract classes.

We can get the set of child-specific names for testing by subtracting the set of parent attributes from the set of child-attributes?

Just an idea, lmk what you think and I take take a stab at implementing it.

[fkiraly]

Hm, that would take care of these not being in the constructor.

It would not cover the "accident" cases, or would it? Where a non-hyper-parameter attribute is being written to in one of the methods. How would we detect this?

[fkiraly]

ie we could exclude any Base classes (or HeterogenousMetaEstimator) from the tests

They are already excluded, since the collections is done via all_estimators, which excludes base classes and abstract mixins (by naming convention, i.e., "starts with Base etc)

[fkiraly]

@ltsaprounis, can your mock estimator perhaps notice which attributes are being written to?

[miraep8]

Alternative/potentially hacky solution......

In Base Classes/Heterogenous Meta Estimator we could overwrite the setattr() call.

In it we could see where setattr() is being called from by doing the following:

sys._getframe().f_back.f_code.co_name

And put is some checks that calls to setattr for protected attributes can only be made by functions that exist on the level of the abstract class (ie generate an error if a child class tries to setattr for any attributes in our protected list?)

[miraep8]

(and to be fair, I thought we were trying to cover cases where it is in the constructor? that at least seemed to be the error case that we were running into)

[fkiraly]

In it we could see where setattr() is being called from by doing the following:

I did not know you could do that!
Does not seem hacky but principled?

I thought we were trying to cover cases where it is in the constructor? that at least seemed to be the error case that we were running into

In the error case, it was set by the constructor, but not an arg to the constructor. Would that not be missed by your first proposed solution?

[miraep8]

I thought we were trying to cover cases where it is in the constructor? that at least seemed to be the error case that we were running into

In the error case, it was set by the constructor, but not an arg to the constructor. Would that not be missed by your first proposed solution?

Hmm, yes, revisiting my original solution I do think it wouldn't work!