Skip to content
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

Store repeatedly used variables as estimator attributes #47 #48

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

grovduck
Copy link
Member

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.

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 grovduck added bug Something isn't working estimator Related to one or more estimators labels Sep 11, 2023
@grovduck grovduck added this to the Core Estimators milestone Sep 11, 2023
@grovduck grovduck self-assigned this Sep 11, 2023
Copy link
Contributor

@aazuspan aazuspan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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.

@grovduck
Copy link
Member Author

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.

@grovduck grovduck merged commit a40d537 into fb_add_estimators Sep 11, 2023
10 checks passed
@grovduck grovduck linked an issue Sep 11, 2023 that may be closed by this pull request
@grovduck grovduck deleted the estimator-attributes branch September 11, 2023 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working estimator Related to one or more estimators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using properties as accessors in CCA/CCorA leads to slow run times
2 participants