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

Update Docstrings in Visualization Module #115

Merged

Conversation

henrifroese
Copy link
Collaborator

  • Fix spelling / too long lines; add hyperlinks; improve explainations

  • Add type information in signature (s: pd.Series), ...

  • Improve module docstring at the very top

The other (functional) changes to scatterplot that were implemented in #107 (overhauling the function + adding 3D support + adding hover_name argument) will be in a separate PR once this is merged.

* Add parameters and their description/type

* Add examples

Co-authored-by: Maximilian Krahn <[email protected]>
@jbesomi
Copy link
Owner

jbesomi commented Jul 23, 2020

Looks great, thank you! 💪

Review:

  1. Example should be kept short and similar to the rest of the docexamples:
    1. one-line to load data into Pandas
    2. use pd.pipe when applicable
    3. n_components=3 -> as of now, pca with three parameters would fail or at least not work as expected

@jbesomi jbesomi marked this pull request as draft July 23, 2020 09:00
@henrifroese
Copy link
Collaborator Author

Thanks, just did it. I believe the example is now as short as it can be while still showcasing the whole functionality.

@jbesomi jbesomi marked this pull request as ready for review July 24, 2020 09:46
@jbesomi
Copy link
Owner

jbesomi commented Jul 24, 2020

Looks good! Can you just resolve the conflicts, please? Then I will merge it 💪

@henrifroese
Copy link
Collaborator Author

Can you just resolve the conflicts, please?

Done

@jbesomi jbesomi merged commit 9ce86ce into jbesomi:master Jul 27, 2020
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