Avoid hasattr(est, expensive-to-calculate-attribute)

[jnothman]

#7481 raises the issue that checking the presence of some attributes can be expensive and #7487 fixed this for feature_importances_. We should identify what other attributes (particularly underscore-suffixed) are currently being checked with hasattr in the codebase and identify any that are risky by way of being defined as properties that are or may be expensive to compute.

grep hackers welcome.

[jnothman]

ping @fukatani in case you're interested.

[fukatani]

OK! I'm interested in this issue, I'll work on it!

I think @property is not appropriate for expensive method primarily.
I'll also check it.

[jnothman]

It's not. But some things can't be changed that easily!

On 26 September 2016 at 22:07, fukatani notifications@github.com wrote:

OK! I'm interested in this issue, I'll work it!

I think @Property is not appropriate for expensive method primarily.
I'll also check it.

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

[fukatani]

I'm sorry for slowly response.
First I extracted all hasattr calling in scikit learn.
And calling in test code is removed.

Result is here.
https://docs.google.com/spreadsheets/d/1PYXnP5vf4ahEuXV-fUG-EuAEc1pjJ_z5KngcXQDp-jg/pubhtml?gid=0&single=true

[fukatani]

Next, I extract all attributes which @property is defined.
And I also check these attributes existence is checked by hasattr.

https://docs.google.com/spreadsheets/d/1PYXnP5vf4ahEuXV-fUG-EuAEc1pjJ_z5KngcXQDp-jg/pubhtml?gid=1304236032&single=true

[fukatani]

So next, we have to check @property calculation cost which attribute is checked by hasattr.
Listed here.

• multiclass.py(379) coef_
• pipeline.py(510) classes_
• cluster\hierarchical.py(839) fit_predict
• ensemble\voting_classifier.py(213) predict_proba
• linear_model\ridge.py(797) classes_
• linear_model\ridge.py(1320) classes_
• linear_model\stochastic_gradient.py(726) predict_proba
• linear_model\stochastic_gradient.py(806) predict_log_proba
• neural_network\multilayer_perceptron.py(629) partial_fit
• neural_network\multilayer_perceptron.py(952) partial_fit
• svm\base.py(482) coef_
• svm\base.py(588) predict_proba
• svm\base.py(627) predict_log_proba

[jnothman]

classes_ should be trivial. predict* and partial_fit must be designed in a way that they're cheap, as they're only expensive once they've got arguments.

It's worth checking the coef_s but they're likely to not be outrageously expensive.

[jnothman]

thanks

[fukatani]

I got it.

[adrinjalali]

Thinking about this issue and going through some of the instances of hasattr(..., 'classes_'), I've got a question: is the task to change all such instances of hasattr into getattr regardless of where they are, or should it be only if we know the attribute is a @property?

For instance, forest.py has such an instance of a call to hasattr, but there the classes_ is not a @property so it's okay and it's cheap. However, it'll be easier to just change all such calls just in case.

[jnothman]

I'm not sure if there are any substantial instances of this outstanding. I'd not be worried about hasattr with classes_. Basically, it's an issue where we have inappropriately designed something as an attribute that should have been a method...

…

On 27 July 2018 at 02:55, Adrin Jalali ***@***.***> wrote: Thinking about this issue and going through some of the instances of hasattr(..., 'classes_'), I've got a question: is the task to change *all* such instances of hasattr into getattr regardless of where they are, or should it be only if we know the attribute is a @Property? For instance, forest.py has such an instance of a call to hasattr, but there the classes_ is not a @Property so it's okay and it's cheap. However, it'll be easier to just change all such calls just in case. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7491 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz68uKpNKMbvHTofJ5Y14hsxpqGhzDks5uKfR2gaJpZM4KGZHj> .

[adrinjalali]

In that case, I guess there isn't much to do (see #11698)

[jnothman]

Thanks for following this up, @adrinjalali