Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ational charts to supported plots
There was a problem hiding this 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.
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. |
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 |
examples/specialty_plots/ishikawa.py Outdated
| Maximum recursion level set. The default is 4. | ||
| DEBUG : bool, optional | ||
| debug : bool, optional |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
examples/specialty_plots/ishikawa.py Outdated
| fig.savefig(output_path, |
There was a problem hiding this comment.
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__.
There was a problem hiding this comment.
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!
examples/specialty_plots/ishikawa.py Outdated
| ''' | ||
| def ishikawaplot(data, figsize=(20, 10), left_margin: float = 0.05, right_margin: float = 0.05, alpha_ps: float = 60.0, |
There was a problem hiding this comment.
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...
.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 | |||
There was a problem hiding this comment.
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.
if you install |
examples/specialty_plots/ishikawa.py Outdated
| Ishikawa Diagrams | ||
| =============== | ||
| Ishikawa Diagrams are useful for visualizing the effect to many cause relationships |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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: ***@***.***> |
- 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
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, |
Uh oh!
There was an error while loading.Please reload this page.
scottshambaugh commentedFeb 21, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
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 |
examples/specialty_plots/ishikawa.py Outdated
| return None | ||
| def ishikawaplot(data, figsize=(20, 10), left_margin: float = 0.05, |
There was a problem hiding this comment.
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.
Disable discarded animation warning on save
Pin sphinx themes more strictly
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
Clean up Curve ArrowStyle docs
"Inactive" workflow: bump operations-per-run
…ational charts to supported plots
- 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 commentedMar 4, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
Hi@rbt94 - it looks like something went wrong with a rebase? Do you need help fixing this or moving the PR forward? |
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 |
@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! |
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: ***@***.***> |
…arts to supported plots
PR Summary
PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst