Skip to content

Adding examples for Encoding Module - #547

Merged
solegalli merged 4 commits into
feature-engine:mainfrom
datacubeR:encoding-examples
Nov 7, 2022
Merged

Adding examples for Encoding Module #547
solegalli merged 4 commits into
feature-engine:mainfrom
datacubeR:encoding-examples

Conversation

@datacubeR

Copy link
Copy Markdown
Contributor

Hi @solegalli

I just added examples for the encoding module.

  • I would like your feedback on this especially because I'm not sure how to test this examples are ok. Is there any way to render a dev documentation to see it?
  • Second, scikit-learn shows some kind of glimpse of the answer, but they use np.arrays so it's a bit more concise than showing a glimpse of a dataframe, what do you think about that?

Please let me know if we are OK with this before start working on other modules.

Finally, do you think it could be a good a idea to have some notebooks for every module with this examples?

Thanks,

Alfonso

@solegalli solegalli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @datacubeR

great job, thank you!

Here are guidelines to build the docs. After these 2 commands, you should see a folder called "build" and in there, you can double click on the file called "index" to render the docs in a browser.

I think, the only thing I would add is a display of the transformed dataframe? I think that may help users to visually understand what the output of the encoding is meant to be.

We do have notebooks with examples in this repo: https://github.com/feature-engine/feature-engine-examples

And we link to them from the user guide. Not sure if they need updating after the various releases of Feature-engine.

Also, let's make one PR per module (this was probably your idea already?). It helps with the review and also to link with other PRs that may modify some of the same files.

Thanks a lot for your contribution!

Comment thread feature_engine/encoding/count_frequency.py
Comment thread feature_engine/encoding/decision_tree.py Outdated
Comment thread feature_engine/encoding/mean_encoding.py Outdated
@datacubeR

datacubeR commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

@solegalli
I rendered a local vervion of the docs to check how it is being rendered. I get this:

image
So it can be seen that >>> doesn't work like in Scikit-Learn (see this) allowing to hide the prompts.

I think Scikit-Learn uses something like this.

I found this site with sphinx extensions https://sphinx-extensions.readthedocs.io/en/latest/sphinx-prompt.html, but no sure if you would like to add something like this or just get rid of the >>>.

@solegalli

Copy link
Copy Markdown
Collaborator

Hi @datacubeR

Yes, I am aware that you need to add some functionality to be able to remove the >>>.

Could you create an issue with the link?

I'd say, leave the >>> in, and we'll take care of adding the functionality to remove them, in a different PR.

Thank you!

@solegalli

Copy link
Copy Markdown
Collaborator

Or actually, when you are done with the code for this PR, I can try adding the functionality, before merging, because it seems fairly straightforward.

Let me know when you finish here, and I can give it a go.

@codecov

codecov Bot commented Nov 1, 2022

Copy link
Copy Markdown

Codecov Report

Merging #547 (fc40473) into main (50cecc5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #547   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files          90       90           
  Lines        3480     3480           
  Branches      681      681           
=======================================
  Hits         3392     3392           
  Misses         32       32           
  Partials       56       56           
Impacted Files Coverage Δ
feature_engine/encoding/count_frequency.py 96.87% <ø> (ø)
feature_engine/encoding/decision_tree.py 96.07% <ø> (ø)
feature_engine/encoding/mean_encoding.py 100.00% <ø> (ø)
feature_engine/encoding/one_hot.py 100.00% <ø> (ø)
feature_engine/encoding/ordinal.py 97.22% <ø> (ø)
feature_engine/encoding/rare_label.py 100.00% <ø> (ø)
feature_engine/encoding/similarity_encoder.py 98.63% <ø> (ø)
feature_engine/encoding/woe.py 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@datacubeR

Copy link
Copy Markdown
Contributor Author

@solegalli this one is ready for review. If OK, I can move on with other modules (in different PRs).

@solegalli

Copy link
Copy Markdown
Collaborator

Hi @datacubeR

Yes, this is good to merge.

I made a PR to your repo: datacubeR#1 to add the button to remove the >>>

If you merge that PR, then I go ahead and merge here.

I look forward to your examples for the other modules :)

Thanks a lot!

@datacubeR

Copy link
Copy Markdown
Contributor Author

Hi @solegalli quick question, to check the resulting copy button. My local branch is now 1 commit behind because of this PR merge. What is the proper way to update local branch with this updated one? Is it just a rebase? how should that be applied?

@solegalli

Copy link
Copy Markdown
Collaborator

You mean to sync your feature branch with the PR I made to your repo?

I think, after you merge the PR, if you do git pull origin feature-branch-name should work. But you won't need that anymore, because I am merging as we speak :)

Thanks a lot for the contribution @datacubeR !

@solegalli
solegalli merged commit 5e0bf37 into feature-engine:main Nov 7, 2022
@solegalli solegalli mentioned this pull request Dec 7, 2022
11 tasks
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