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

StandardScaling with separate constructor and fit methods to replace MeanStdScaling #107

Merged
merged 16 commits into from
May 17, 2022

Conversation

mzgubic
Copy link
Contributor

@mzgubic mzgubic commented Feb 28, 2022

Closes #106, closes #59, closes #87

This is made non-breaking by deprecating the MeanStdScaling.

To do:

  • mention fit! in the docs

src/scaling.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #107 (fdbe463) into main (1c86cbd) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   98.74%   98.88%   +0.14%     
==========================================
  Files          13       15       +2     
  Lines         159      179      +20     
==========================================
+ Hits          157      177      +20     
  Misses          2        2              
Impacted Files Coverage Δ
src/FeatureTransforms.jl 100.00% <ø> (ø)
src/deprecated.jl 100.00% <100.00%> (ø)
src/fit.jl 100.00% <100.00%> (ø)
src/scaling.jl 95.65% <100.00%> (+1.53%) ⬆️
src/apply.jl 100.00% <0.00%> (ø)
src/periodic.jl 100.00% <0.00%> (ø)
src/one_hot_encoding.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c86cbd...fdbe463. Read the comment docs.

src/scaling.jl Outdated Show resolved Hide resolved
docs/src/examples.md Outdated Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
src/scaling.jl Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
docs/Project.toml Outdated Show resolved Hide resolved
src/FeatureTransforms.jl Outdated Show resolved Hide resolved
src/fit.jl Outdated Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
src/scaling.jl Show resolved Hide resolved
src/scaling.jl Outdated Show resolved Hide resolved
@glennmoy
Copy link
Member

In general I'm in favour of these changes - I just don't think we should extend StatsBase without good reason, and I'd prefer go with a Union type in the constructor.

Oh and we should also document fit! as part of the API and expect stateful transforms to have a method for it as well as apply.

@mzgubic mzgubic changed the title RFC: StandardScaling with separate constructor and fit methods to replace MeanStdScaling StandardScaling with separate constructor and fit methods to replace MeanStdScaling May 16, 2022
@mzgubic
Copy link
Contributor Author

mzgubic commented May 16, 2022

Thank you both for reviewing, I think it's ready to go now. We went with the mutable approach so as not to inflate the API with an extra Fit class.

docs/src/transforms.md Outdated Show resolved Hide resolved
@mzgubic mzgubic merged commit 4a636c8 into main May 17, 2022
@mzgubic mzgubic deleted the mz/standardscaling branch May 17, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants