Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

added Ishikawa plot in response to issue #25222 add organizational ch…#25248

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
rbt94 wants to merge62 commits intomatplotlib:mainfromrbt94:my-feature

Conversation

rbt94
Copy link

…arts to supported plots

PR Summary

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (andpytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
  • API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@rcomer
Copy link
Member

Thank you for your contribution@rbt94! It looks like there are some style problems that need fixing, shown by the automated checks. So I'm going to mark this as "draft" for now. Please mark it as "ready for review" when you are ready. In the meantime, if you need help, feel free to ask questions here. Or you may prefer to ask them in ourincubator gitter room.

@rcomerrcomer marked this pull request as draftFebruary 18, 2023 18:01
@rcomerrcomer linked an issueFeb 18, 2023 that may beclosed by this pull request
@rbt94rbt94 marked this pull request as ready for reviewFebruary 19, 2023 10:35
@rbt94
Copy link
Author

sorry@rcomer I have modified the code and I wanted to rerun the github checks before "marking for review" the code. Could you please describe to me the procedure before actually review the code? Sorry but I am pretty new to github. Thanks a lot

Maximum recursion level set. The default is 4.
DEBUG : bool, optional
debug : bool, optional
Copy link
Member

Choose a reason for hiding this comment

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

Suggest use the logging module instead of manual debugging.

rbt94 reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

using the logging you can also emit to the logger, but it the log level is set higher it will be automatically dropped so you do not need to thread this parameter through.

jklymak reacted with thumbs up emoji

fig.savefig(output_path,
Copy link
Member

Choose a reason for hiding this comment

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

Please do not add savefig calls in examples. Don't need show, and probably don't need to encapsulate in a__main__.

rbt94 reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

To elaborate a bit more, the way our documentation build system works is we run the examples and then find and show any figures that are created.

While using'__main__' is a good practice for stand-alone scripts, will effectively hide the example from the docs which will in turn prevent the rendered image from showing up in the build docs!

rbt94 reacted with thumbs up emoji
'''


def ishikawaplot(data, figsize=(20, 10), left_margin: float = 0.05, right_margin: float = 0.05, alpha_ps: float = 60.0,
Copy link
Member

Choose a reason for hiding this comment

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

We have an 88-character line limit. Please wrap...

rbt94 reacted with thumbs up emoji
.flake8 Outdated
@@ -92,6 +92,7 @@ per-file-ignores =
examples/lines_bars_and_markers/marker_reference.py: E402
examples/misc/print_stdout_sgskip.py: E402
examples/misc/table_demo.py: E201
examples/specialty_plots/ishikawa.py: E501
Copy link
Member

Choose a reason for hiding this comment

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

Please wrap the example instead of adding an exception for it.

@tacaswell
Copy link
Member

I have modified the code and I wanted to rerun the github checks before "marking for review" the code.

if you installflake8 and invoke it from the top level of our docs you should catch most of the formatting issues. To be more through you can also install pre-commit (seehttps://matplotlib.org/devdocs/devel/development_setup.html#install-pre-commit-hooks-optional) which will run the exact same checks locally for you.

@tacaswelltacaswell added this to thev3.8.0 milestoneFeb 20, 2023
Ishikawa Diagrams
===============

Ishikawa Diagrams are useful for visualizing the effect to many cause relationships
Copy link
Contributor

Choose a reason for hiding this comment

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

Wikipediasays that these are also known as "fishbone diagrams, herringbone diagrams, and cause-and-effect diagrams". Could you add those alternate names to the description so they show up in a search?

jklymak and rbt94 reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

It may also be good to link to Wikipedia so that readers can find additional information on this plot type.

rbt94 reacted with thumbs up emoji
@rbt94
Copy link
Author

rbt94 commentedFeb 20, 2023 via email

Thank you guys, in a few days I will revise the code with your comments andrun the checks also locally.Best,RobertIl lun 20 feb 2023, 16:57 Scott Shambaugh ***@***.***> hascritto:
…
***@***.**** commented on this pull request. ------------------------------ In examples/specialty_plots/ishikawa.py <#25248 (comment)> : > @@ -0,0 +1,228 @@ +""" +=============== +Ishikawa Diagrams +=============== + +Ishikawa Diagrams are useful for visualizing the effect to many cause relationships Wikipedia says <https://en.wikipedia.org/wiki/Ishikawa_diagram> that these are also known as "fishbone diagrams, herringbone diagrams, and cause-and-effect diagrams". Could you add those alternate names to the description so they show up in a search? — Reply to this email directly, view it on GitHub <#25248 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVDSMTIT3CCW4GFTUYVG4HTWYOH6TANCNFSM6AAAAAAVAGT6GU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

@jklymakjklymak marked this pull request as draftFebruary 20, 2023 21:58
- improved example description docstring- removed __main__ call- debug print converted into logging.debug strings- removed sohw fig and save fig and related Path library- removed exception from flake8 conf file
@rbt94
Copy link
Author

Hello @matplotlib/developers I have implemented the reported modifications and passed the pre-commit checks before committing. Unfortunately I cannot reach a successful test of the entire commit. Can you please help me by indicating what else needs to be fixed in order to pass the test? Is it the sphinx warning "Title overline too short" or something else?

Thank you,
Robert

@scottshambaugh
Copy link
Contributor

scottshambaugh commentedFeb 21, 2023•
edited
Loading

I left a comment indicating how to fix the sphinx build error. I believe the Main Pytest Windows_py311 failure is a random CI timeout, let's see if it passes when you push up that change and it runs again. If it's consistent then I believe it's a separate issue from your code changes that we'll have to look into.

rbt94 reacted with thumbs up emoji

@rbt94
Copy link
Author

Thank you@scottshambaugh. It is now sufficient to mark for review or there are any other issue to fix? I do not know how codecov works.

Thanks

return None


def ishikawaplot(data, figsize=(20, 10), left_margin: float = 0.05,
Copy link
Member

Choose a reason for hiding this comment

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

This method really just acts on a single axes. Strongly suggest re-structuring withoutfigsize and just passing in an axes. If no axes is passed in, then make a new figure, and return the new axes.

rbt94 reacted with thumbs up emoji
tacaswelland others added18 commitsMarch 2, 2023 12:34
By combining A/B parameters, to match the style of the CurveBracket,CurveFilled, etc.Also, tweak the example image so that it is not scaled down in the docs,and explicitly shows 0°.
BLD: Pre-download Qhull license to put in wheels
Tk: Fix size of spacers when changing display DPI
"Inactive" workflow: bump operations-per-run
- improved example description docstring- removed __main__ call- debug print converted into logging.debug strings- removed sohw fig and save fig and related Path library- removed exception from flake8 conf file
- added wikipedia link to ishikawa desc doc string- reformatted function def to work on axes object- fix plt.tight_layout leaving the user to choose- reduced fisize still mantaining a decent rendering- added swotplot.py for SWOT plots with the same logic, design pattern  and style
@rbt94
Copy link
Author

rbt94 commentedMar 4, 2023•
edited
Loading

I'm not sure that the input data on either of these functions needs to be a dictionary. The names and data are paired, but don't seem to be inherently dictionary-like.

The logic and formatting I have used was to resemble a plain JSON file format, just to speed-up operations in a larger application.

- integrated hypot call instead sqrt formula- fix dict iteration- debug eliminated- alpha vars renaming- docstrings minor fixes
@melissawm
Copy link
Member

Hi@rbt94 - it looks like something went wrong with a rebase? Do you need help fixing this or moving the PR forward?

@rbt94
Copy link
Author

rbt94 commentedApr 8, 2023 via email

Thank you Melissa it would be very appreciated. Currently I have appliedfor a second job and I have not so spare time to dedicate into this. Sorryabout that.<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>Privodi virus.www.avast.com<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail><#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>Il giorno mer 29 mar 2023 alle ore 14:00 Melissa Weber Mendonça <***@***.***> ha scritto:
…
Hi@rbt94 <https://github.com/rbt94> - it looks like something went wrong with a rebase? Do you need help fixing this or moving the PR forward? — Reply to this email directly, view it on GitHub <#25248 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVDSMTMFNKVXWX5P72TNBPDW6QP43ANCNFSM6AAAAAAVAGT6GU> . You are receiving this because you were mentioned.Message ID: ***@***.***>
-- Bodo Roberto

@melissawm
Copy link
Member

@rbt94 no worries, would you prefer trying to solve this or could another contributor take over your PR, as long as they keep you as a co-author? Cheers!

@rbt94
Copy link
Author

@rbt94 no worries, would you prefer trying to solve this or could another contributor take over your PR, as long as they keep you as a co-author? Cheers!

Thank you@melissawm It would be perfect for me. Cheers!

@QuLogic
Copy link
Member

Replaced by#26064, IIUC. Thank you for your initial work on this,@rbt94.

@QuLogicQuLogic closed thisJun 6, 2023
@rbt94
Copy link
Author

rbt94 commentedJun 7, 2023 via email

Thank you too for your work, sorry for the inconvenience for paused PR.Best,RobertIl mar 6 giu 2023, 22:39 Elliott Sales de Andrade ***@***.***>ha scritto:
…
Replaced by#26064 <#26064>, IIUC. Thank you for your initial work on this,@rbt94 <https://github.com/rbt94>. — Reply to this email directly, view it on GitHub <#25248 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVDSMTKEIG3Q4YHTGQDE5KDXJ6IQLANCNFSM6AAAAAAVAGT6GU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

@QuLogicQuLogicQuLogic left review comments

@jklymakjklymakjklymak left review comments

@timhoffmtimhoffmtimhoffm left review comments

@scottshambaughscottshambaughscottshambaugh left review comments

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

Assignees
No one assigned
Projects
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.

[ENH]: add organizational charts to supported plots
15 participants
@rbt94@rcomer@tacaswell@scottshambaugh@QuLogic@jklymak@melissawm@timhoffm@devRD@anntzer@oscargus@xtanion@greglucas@ianhi@ksunden
[8]ページ先頭
©2009-2025 Movatter.jp