8000 DOC Added linestyle into plot_lda example by star1327p · Pull Request #23869 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

DOC Added linestyle into plot_lda example #23869

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jul 20, 2022
Merged

DOC Added linestyle into plot_lda example #23869

merged 7 commits into from
Jul 20, 2022

Conversation

star1327p
Copy link
Contributor

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Added linestyle changes, so that the graph is easier to read for people with color blindness.

Any other comments?

The legend covers part of the yellow trend, so I hope this can also be fixed in the future.

star1327p added 3 commits July 6, 2022 17:51
@star1327p
Replace "consequences" with "advantages"
903d1c1 
The word "consequence" usually means something negative, so it's better to replace it with "advantage" to refer to the positive outcomes.
@star1327p
Merge branch 'scikit-learn:main' into main
b4afb3f
@star1327p
Added linestyle changes
e6a2306 
Linestyle changes make the graph easier to read for people with color blindness.
@Micky774
Copy link
Contributor

Looks like the linter is unhappy -- could you run black on your changed files and push?

@star1327p
Copy link
Contributor Author

Looks like the linter is unhappy -- could you run black on your changed files and push?

Yes, I have run black, formatted the changed files, and pushed to the repo. Now all checks have passed.

@Micky774 Micky774 added the Quick Review For PRs that are quick to revie 8000 w label Jul 11, 2022
@adrinjalali
Copy link
Member

So here the "Check the rendered docs here!" job is not visible because @star1327p is sending the PR from main to main. You should always create a branch to work on a PR. For this PR, it can be closed and a new one could be created using a non-main branch for us to check the rendered version.

This is the error message on the relevant action:

 ++ curl -H 'Accept: application/vnd.github.v3+json' -H 'Authorization: token ***' https://api.github.com/repos/star1327p/scikit-learn/commits/b34ff174f1603416b9b9e9fa7dbb410ee1be7d08/pulls
++ jq '.[0].number'
+ PULL_REQUEST_NUMBER=null
+ BRANCH=pull/null/head
+ curl --request POST --url https://circleci.com/api/v2/project/gh/scikit-learn/scikit-learn/pipeline --header 'Circle-Token: ***' --header 'content-type: application/json' --header 'x-attribution-actor-id: github_actions' --header 'x-attribution-login: github_actions' --data '{"branch":"pull/null/head","parameters":{"GITHUB_RUN_URL":"https://nightly.link/scikit-learn/scikit-learn/actions/runs/2685409653"}}'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
{
  "message" : "Branch not found"
100   168  100    36  100   132     67    247 --:--:-- --:--:-- --:--:--   314
}

@ArturoAmorQ
Copy link
Member

Thanks for the contribution @star1327p, it LGTM as it is.

If you feel like going an extra step, feel free to implement the notebook style as intended in #22406 i.e., use # %% as cell separator where you think it is appropriated, add titles/subtitles and move the imports to the first cell where they are required.

Look at existing notebook-style example for inspiration e.g. main/examples/manifold/plot_swissroll.py with its rendered HTML in the doc.

@jjerphan jjerphan changed the title Added linestyle changes DOC Added linestyle changes Jul 17, 2022
@thomasjpfan thomasjpfan mentioned this pull request Jul 17, 2022
Merged
thomasjpfan
thomasjpfan approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

I'm happy with this PR as is. We can improve it with follow up PRs.

@thomasjpfan thomasjpfan changed the title DOC Added linestyle changes DOC Added linestyle into plot_lda example Jul 19, 2022
ArturoAmorQ
ArturoAmorQ approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member
@ArturoAmorQ ArturoAmorQ left a comment

Choose a reason for hiding this comment

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

I agree with @thomasjpfan that this PR is good as it is.

@adrinjalali adrinjalali merged commit 26800e9 into scikit-learn:main Jul 20, 2022
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Aug 4, 2022
@star1327p @thomasjpfan @glemaitre
DOC Added linestyle into plot_lda example (scikit-learn#23869)
9324c52 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
glemaitre added a commit that referenced this pull request Aug 5, 2022
@star1327p @thomasjpfan @glemaitre
DOC Added linestyle into plot_lda example (#23869)
c1457b2 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@ArturoAmorQ ArturoAmorQ ArturoAmorQ approved these changes

Assignees
No one assigned
Labels
Documentation Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@star1327p @Micky774 @adrinjalali @ArturoAmorQ @thomasjpfan @glemaitre
0