-
-
Notifications
You must be signed in to change notification settings - Fork 312
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
Mean Normalisation Scaling #806
Mean Normalisation Scaling #806
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #806 +/- ##
==========================================
+ Coverage 97.98% 98.00% +0.01%
==========================================
Files 107 109 +2
Lines 4320 4350 +30
Branches 857 709 -148
==========================================
+ Hits 4233 4263 +30
Misses 54 54
Partials 33 33 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @VascoSch92
This is looking really good. Thank you for the first draft.
I think we could tidy it a bit so that we don't loop neither in fit nor in transform.
Could you take a look?
Thank you!
Hey @solegalli I addressed your comments. Please let me know what you think :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @VascoSch92
This is looking really good. The tests are great.
Regarding the logic, I think we can speed this up if we store the range instead of min max,, and if we use dictionaries instead of dataframes. Could you give that a go?
Feel free to start working on the dosctrings and on adding a user guide :)
Hey @solegalli I changed what requested.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @VascoSch92 really good work here. Thank you so much!
We need to add a few files to create the docs now. Would you be able to do that as well?
Thanks a lot for the hard work.
Last but not least: we need to add the new module on the readme and on the frontpage of the documentation, which lives here: https://github.com/feature-engine/feature_engine/blob/main/docs/index.rst Thank you!! |
Hey @solegalli
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @VascoSch92 thanks for the quick turnaround. The api docs look great.
Co-authored-by: Soledad Galli <[email protected]>
Co-authored-by: Soledad Galli <[email protected]>
Co-authored-by: Soledad Galli <[email protected]>
Co-authored-by: Soledad Galli <[email protected]>
Co-authored-by: Soledad Galli <[email protected]>
Hey @solegalli I changed to dictionaries instead of |
Amazing! Thanks a lot! We just need to add a description / demo to the user guide folder in the docs and we are good to go then :) |
let me give a look ;-) |
@solegalli quick question: in general feature engine use scaling transformers from sklearn. Should we also include examples of these transformers? |
Hey @solegalli I updated the docs with a demo. Let me know what do you think. It is just a first version :-) |
Hey @solegalli :-) did you have time to look at the latest changes? |
Pending acceptance of suggested changes: VascoSch92#1 |
…ormalization rewords the documentation and adds missing links
First version of the
MeanNormalizationScaling
as discussed in #763I create a new module scaling as discussed.
Probably, you will also add new scaling in this module.