search

lemma-osu / sknnr

scikit-learn compatible estimators for various kNN imputation methods
https://sknnr.readthedocs.io
0 stars 1 forks source link

Store repeatedly used variables as estimator attributes #47 #48

Closed grovduck closed 1 year ago

grovduck commented 1 year ago

For CCATransformer and CCorATransformer, store matrices from ordination as estimator attributes within fit method. Previously, we were calling properties from CCA and CCorA (respectively) that were recomputing expensive operations. Because the matrix projectors don't change once fit, it's more efficient to store as estimator attributes.

Also, remove redundant call to _validate_data in TransformedKNeighborsMixin._apply_transform as the transformers are now responsible for that call.

grovduck commented 1 year ago

I know there are still repeated property calls in the underlying transformer implementations (e.g. the ConstrainedOrdination.env_center property gets recalculated multiple times) that could be simplified with the same strategy as this and probably improve performance further. I'd say those could be included in this PR, but getting this merged quick to keep things moving sounds reasonable to me.

I absolutely agree that there is more work to be done on CCA and CCorA. I'd like to defer that for now, but have created #49 to track this. Merging now to keep things moving seems like a good tack to me.