-
-
Notifications
You must be signed in to change notification settings - Fork 7.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code 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 our incubator 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.
Wikipedia says 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 and
run the checks also locally.
Best,
Robert
Il lun 20 feb 2023, 16:57 Scott Shambaugh ***@***.***> ha
scritto:
… ***@***.**** 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, |
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 without figsize
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
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 applied
for a second job and I have not so spare time to dedicate into this. Sorry
about that.
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
Privo
di 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,
Robert
Il 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
pytest
passes)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.rst
ornext_api_changes/README.rst