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

TickedStroke, a stroke style with ticks useful for depicting constraints#15458

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

Merged
QuLogic merged 1 commit intomatplotlib:masterfromramcdona:TickedStroke
Aug 6, 2020
Merged

Conversation

ramcdona
Copy link
Contributor

In optimization, constraints are frequently plotted with a ticked line -- where
the ticks indicate that one side of the line is invalid. TickedStroke implements
a PathEffect to draw lines with a ticked style. The spacing, length, and angle
of ticks can be controlled.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak
Copy link
Member

Thanks a lot for the PR. Can you give us an example? If this is to go in, it will need an example in the gallery and tests. OTOH maybe a good idea to get some input as to whether this will be of general enough interest to include in the core library.

@ramcdona
Copy link
ContributorAuthor

Unfortunately, I haven't been able to get self-hosting matplotlib working, so I didn't have the confidence to include my tests or dig into the gallery code.

I didpost a test script and example of output to the devel mailing list.

I've implemented essentially the same thing twice before - in Java andMatlab. The Java Graphics2D implementation is most similar to this one, but I can't post it online.

There is anotheropen-source implementation of this idea inspired by my Matlab implementation above, but it is not integrated as a PathEffect and doesn't understand the plot's aspect ratio -- so the results are not aspretty.

Overall, I believe this would be reasonably frequently used and it presents little risk of any maintenance burden in the future.

Even if it was not used frequently, I believe it warrants inclusion to serve as an example of what can be accomplished by building on PathEffect.

@jklymak
Copy link
Member

Please include your script above in the description and the plot. The mailing list seems to have eaten your*.py and some folks on here are not subscribed to the mailing list.

In the end you will have to at least make tests, or find someone to do it for you.

@ramcdona
Copy link
ContributorAuthor

Where is the preferred place to add appropriate demos / tests?

Is it the examples gallery? Such as the briefPathEffects demo. Or perhaps up with the line styles, or the contour plotting?

Or is it in the tests directory? There is aPathEffect test there too. However, since I was unable to bootstrap matplotlib, I have not yet gotten these tests to run -- my attempts all crash with a warning about deprecation of ImageComparisonTest and some error about nose. No idea what to do from there -- it looks like matplotlib deprecated its own test harness...

@jklymak
Copy link
Member

You could hit us up for more guidance on discourse.matplotlib.com. Not sure what the issue is, but the test definitely run, so there is something missing or incompatible in your install.

@anntzer
Copy link
Contributor

The idea looks nice (dunno if it should go to the library -- in which case some tests are needed -- or just be an example, in which case you can skip that but some more docs/examples would still be nice).

@ramcdona
Copy link
ContributorAuthor

@anntzer Thanks for all the style suggestions -- all have been incorporated.

I ended up having to use np.int(np.ceil(x)) instead of math.ceil(x) to ensure an integer type for indexing. If there is a better way to do this, I'm all ears.

@ramcdona
Copy link
ContributorAuthor

@jklymak I think I beat on my local setup enough to run tests and demos. It'll take a couple days to put something pretty together. Thanks for the feedback.

@anntzer
Copy link
Contributor

Ah yes, I forgot about the float/int problem. I tend to usemath.ceil in that case (yes, contradicting my earlier suggestion to just use np), but with the explicitly prefixed name.int(np.ceil(...)) is fine too (though no need fornp.int).

@ramcdona
Copy link
ContributorAuthor

I've pushed up a first attempt at some examples. I still don't have a local matplotlib dev environment set up, so there is a chance I missed something when I pulled this together.
Feedback appreciated.

@ramcdona
Copy link
ContributorAuthor

For the interested, the example now builds with the resulthere.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

There are some formal errors in the docstring causing sphinx to complain:

/home/circleci/project/lib/matplotlib/patheffects.py:docstring of matplotlib.patheffects.TickedStroke:50: WARNING: Unexpected indentation./home/circleci/project/lib/matplotlib/patheffects.py:docstring of matplotlib.patheffects.withTickedStroke:43: WARNING: Unexpected indentation.

Please fix these.

@ramcdona
Copy link
ContributorAuthor

There are some formal errors in the docstring causing sphinx to complain:

/home/circleci/project/lib/matplotlib/patheffects.py:docstring of matplotlib.patheffects.TickedStroke:50: WARNING: Unexpected indentation./home/circleci/project/lib/matplotlib/patheffects.py:docstring of matplotlib.patheffects.withTickedStroke:43: WARNING: Unexpected indentation.

Please fix these.

I've fixed all the things you specifically called out, but I'm not sure that covers the above messages -- where did you pull these from? I'd like to see more context as they don't seem to refer to line number in the source, and I don't know what indentation it expects.

@timhoffm
Copy link
Member

They are from the circleCI build output. The numbers are relative to the start of the docstring (flake8 does not know about file line numbers).

@ramcdona
Copy link
ContributorAuthor

They are from the circleCI build output. The numbers are relative to the start of the docstring (flake8 does not know about file line numbers).

I'm still missing something. The docstrings it references don't have nearly that many lines. Perhaps it is characters from the start.

StackOverflow says this is often caused by a missing blank line -- but a root cause is something to do with inheritance of documentation. I certainly don't see what to fix to make this go away.

@ramcdona
Copy link
ContributorAuthor

Finally managed to get doc formatting issue fixed.

Gallery Examples

Docs

timhoffm and JustinSGray reacted with hooray emoji

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Code and docstring look good! Just a few more comments on the example.

@ramcdona
Copy link
ContributorAuthor

@jklymak I've added tests to this PR, I'd appreciate if you review and remove the 'Needs tests' label when you can.

@jklymakjklymak added this to thev3.4.0 milestoneJun 15, 2020
Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me plus or minus some re-organization of where the demo goes and a couple of style things. OTOH I've not looked super carefully at the code to create the ticks, particularly to see if it is as fast as possible, but it looks reasonable to me.

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

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

Just a couple of small things with the test. If I forget, please ping me for a re-review. Thanks for your hard work on this, it seems like a nice feature...

@ramcdona
Copy link
ContributorAuthor

@jklymak Please re-review when you have an opportunity.

Parameters
----------
offset : pair of floats
The offset to apply to the path, in points.
Copy link
Member

Choose a reason for hiding this comment

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

What are the two values? x/y? parallel/perpendicular?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is a verbatim copy of the documentation for this parameter inherited from AbstractPathEffect. See line 30, 301, 360 of this file. It is paraphrased on line 233.

It is an offset in x, y.

Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

One small typo.

@ramcdona
Copy link
ContributorAuthor

Just a couple of small things with the test. If I forget, please ping me for a re-review. Thanks for your hard work on this, it seems like a nice feature...

Thanks for all the constructive feedback.

Looks like effort is pushing towards 3.4.0 now -- do you see anything remaining that would keep this from making that milestone?

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Please rebase on top of master. The CI failure is fixed there.

@ramcdona
Copy link
ContributorAuthor

Please rebase on top of master. The CI failure is fixed there.

Happy to do so. Should I squash it all down to a single commit, or keep this all spread out?

@timhoffm
Copy link
Member

Should I squash it all down to a single commit, or keep this all spread out?

Please squash.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Apart fromthetar and squashing, this is good to go.

@ramcdona
Copy link
ContributorAuthor

Apart fromthetar and squashing, this is good to go.

Thanks much!

Copy link
Member

@QuLogicQuLogic left a comment

Choose a reason for hiding this comment

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

Small corrections.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

@ramcdona please squash one final time.

Thanks for the patience to go through the All our reviews.

…nts.In optimization, constraints are frequently plotted with a ticked line -- wherethe ticks indicate that one side of the line is invalid.  TickedStroke implementsa PathEffect to draw lines with a ticked style.  The spacing, length, and angleof ticks can be controlled.
@QuLogicQuLogic merged commitd38e443 intomatplotlib:masterAug 6, 2020
@QuLogic
Copy link
Member

Thanks@ramcdona! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

@ramcdona
Copy link
ContributorAuthor

Thanks@ramcdona! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

Thanks for working with me to get this in. I know lots of folks who will be glad to see this included.

We'll see if I have another itch to scratch. There are lots of other PathEffects that could be done -- architecture, drafting, cartography, etc. all have interesting line styles that would be likely be implemented this way rather than just a pattern of dash/dots.

jklymak reacted with thumbs up emoji

@ramcdonaramcdona deleted the TickedStroke branchAugust 6, 2020 21:29
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@jklymakjklymakjklymak left review comments

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

5 participants
@ramcdona@jklymak@anntzer@timhoffm@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp