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

Math addition transformer #91

Merged

Conversation

michalgromiec
Copy link
Contributor

issue #69 - implementation of AdditionTransformer from mathematical transformation

Copy link
Collaborator

@solegalli solegalli left a comment

Choose a reason for hiding this comment

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

Hi @michalgromiec thank you again for the great work. Please see my comments. I also think we should change the name of the python script, to maybe mathematical_combination.py.

I think mathematical_transformers.py is too similar to the script variable_transformer.py, which applies math transformations variables per variables, and users may get confused.

Cheers
Sole

feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
feature_engine/mathematical_transformers.py Outdated Show resolved Hide resolved
FIX change transformer name from AdditionTransfomer to MathematicalVariableCombinator
FEAT variables_set_alias attribute added
TESTS: fixing to reflect code changes
feature_engine/mathematical_combination.py Show resolved Hide resolved
feature_engine/mathematical_combination.py Outdated Show resolved Hide resolved
feature_engine/mathematical_combination.py Outdated Show resolved Hide resolved
feature_engine/mathematical_combination.py Outdated Show resolved Hide resolved
feature_engine/mathematical_combination.py Outdated Show resolved Hide resolved
feature_engine/mathematical_combination.py Outdated Show resolved Hide resolved
feature_engine/mathematical_combination.py Outdated Show resolved Hide resolved
feature_engine/mathematical_combination.py Show resolved Hide resolved
feature_engine/mathematical_combination.py Outdated Show resolved Hide resolved
feature_engine/mathematical_combination.py Show resolved Hide resolved
@solegalli
Copy link
Collaborator

Hi @michalgromiec thanks a lot for the amount of work. Could I also ask the following:

Because this is a new feature that adds functionality at the moment not present in Feature-engine we should also add a bit of information to the documentation. Could I also ask you to include the following files:

  • nside the folder docs make a subfolder called mathematical_combination
  • inside the mathematical_combination folder we need an index.rst and another rst file with the name of the transformer you just created where we add details to populate the dosctring of the transformer with sphinx, plus an example of how it can be used (code).
  • in the doc/index.rst we need to update the index to include the mathematical combination functionality under the wrappers
  • in the doc/index.rst, please add themathematical_combination folder to the toctree at the end.

Test that the docs run and build successfully.

Thanks a lot!

michalgromiec and others added 5 commits August 11, 2020 21:48
…maticalVariable

REFACTOR: attribute variables_set_alias renamed to new_variables_names
FEAT: added build of dictionary of new feature names and their operations
FEAT: added check of equal length of operations and new_feature_names
DOCS: updated docstrings for mathematical combinator

TESTS: added tests to cover above changes
fixed rst underline, changed tests name, removed dup test
@solegalli solegalli merged commit fb843c9 into feature-engine:master Aug 12, 2020
@solegalli
Copy link
Collaborator

@michalgromiec all done! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants