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

Simplify fit parameters for CCATransformer and CCorATransformer #31

Merged
merged 1 commit into from
May 31, 2023

Conversation

aazuspan
Copy link
Contributor

This will close #30 by:

  1. Making y a required parameter for fitting CCATransformer and CCorATransformer.
  2. Making GNNRegressor and MSNRegressor choose the appropriate fitting data instead of the transformer.
  3. Renaming spp to y_fit throughout.

Transformers no longer take `spp` parameters when fitting. Instead,
estimators now choose the appropriate `y_fit` data to pass to the
transformer. This simplifies the responsibility of transformers and
avoids some error checking that would have been needed otherwise.

The `spp` parameter to the estimators was renamed to `y_fit` to be more
general.
@aazuspan aazuspan added bug Something isn't working estimator Related to one or more estimators labels May 31, 2023
grovduck added a commit that referenced this pull request May 31, 2023
We exported data from yaImpute for the five core estimators at k=5
for both unweighted (equal-weighting to all neighbors) and weighted
(neighbors are weighted by their inverse distance) predictions of the
y attributes.  The tests assert that the predict method in each
estimator is returning the same values.  Note that the constructor
of each estimator uses a custom function (yaimpute_weights) to the
weights parameter in order to match the weighting scheme of yaImpute.
@grovduck
Copy link
Member

This looks great. Ignore the commit reference above. I was trying to follow your convention of referencing both the issue and PR numbers in the commit title, but I think you sneaked this commit in there before I did ;). I ended up amending my commit message, but alas too late for this.

Let's get this one merged first as I think it will have some breaking changes in #32 with the spp vs y_fit parameter.

@aazuspan
Copy link
Contributor Author

Let's get this one merged first as I think it will have some breaking changes in #32 with the spp vs y_fit parameter.

Sounds good, thanks!

@aazuspan aazuspan merged commit f9a9878 into fb_add_estimators May 31, 2023
@aazuspan aazuspan deleted the simplify_fit branch May 31, 2023 17:08
@grovduck grovduck linked an issue Jun 7, 2023 that may be closed by this pull request
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.

Simplify fit parameters for CCATransformer and CCorATransformer?
2 participants