-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Add maximum streamline length property. #6062
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
Can you add a test for this?
Other than that seems perfectly reasonable and I agree doing the integration direction as a second PR is a good idea. Smaller changes are easier to review.
I will try to set up a short test.
My first interface idea for the integration direction is to change maxlength optionally in a tuple:
maxlength=[0,2] # this would be only forward integration.
maxlength=[-4,0] # only (longer) backward integration.
maxlength=2 # this would be translated to maxlength=[-2,2], which is the old default.
This would give the most flexibility. Or is it too complicated?
The negatives would be confusing. I think a tri-state integration_direction in {'forward', 'reverse', 'both'} and then the maximum_length is either all forward, all back or split evenly would be simpler and get almost as much power.
Ok, i have added a test and copied the interface to axes and pyplot. I hope this was the right thing to do?
Tristate sounds good. Do you have an example, how it should be done in matplotlib?
I think 'backward' is better than 'reverse', since it is the standard term when referring to integrations.
I think just strings are good enough (just validate on the way in).
You will also need to commit the test image see http://matplotlib.org/devel/testing.html#writing-an-image-comparison-test
This should be a png only test I think.
Hi,
thanks for the all your help and quick replies! I now have added two more patches:
- The baseline image for the maxlength test
- A complete patch + test for the integration direction
Let me know, if anything has to be changed and i will try to do it tomorrow.
Ok, after ironing out my rookie mistakes, i have added two more commits.
Now only a single test fails, but it does not seem to be realted to my commits.
Is there anything i can do? Thanks!
edit: Seems like the last problem fixed itself!
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.
All new args need to go at the end to maintain back-compatibility. The reason is if people are (un wisely!) passing in everything as positional args.
When we can drop python2 and can break API again most of these should be converted to kwarg only, but for now they need to go at the end.
Left a few minor comments that need to be addressed. I see what you are doing putting minlength and maxlength together, but we should not break API 😞 .
Could you also add an entry into https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new so that this gets properly advertised in the next release?
Hi, Sorry for the delay. It seems like i don't get any Email notifications. I have hopefully addressed all the issues.
I will try to enable my notifications.
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.
can you re-arrange the docstrings too?
The tests are failing after the merge with upstream. I have to track that down later.
Sorry this got dropped!
Can you rebase instead of merging upstream into your branch?
See http://matplotlib.org/devdocs/devel/gitwash/development_workflow.html?highlight=rebase
Thanks for the hint about a rebase rather than a merge.
But if i now revert the merge commit, i still have the merge/merge-revert in the history, which makes the rebase rather difficult (After conflicts with my changes, i get conflicts because of the merge commit).
Is there an easier way to do it? Sorry to bother you because of this. The easiest way would to make a new branch, and cherry-pick my changes. But i can not change the branch attached to this pull request.
If you do an interactive rebase, you can delete the commits that you don't
want in the branch and then force-push that up to github.
On Fri, Nov 4, 2016 at 7:04 AM, Manuel Jung notifications@github.com
wrote:
Thanks for the hint about a rebase rather than a merge.
But if i now revert the merge commit, i still have the merge/merge-revert
in the history, which makes the rebase rather difficult (After conflicts
with my changes, i get conflicts because of the merge commit).
Is there an easier way to do it? Sorry to bother you because of this. The
easiest way would to make a new branch, and cherry-pick my changes. But i
can not change the branch attached to this pull request.—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6062 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-AWfEMOjbTmqfAsmDjVSw-XVJLTsks5q6xEsgaJpZM4HjPdA
.
Ok, thanks for the hint about the interactive rebase. The rebase is done, but the test (of course) still fail.
I have looked into it for a bit, but i don't think this is (mainly) because of the #6504. The starting/end points seem to be slightly different. I will look into this asap.
If the diff images do not show any issue with the arrows (especially their size), then I think you are right about #6504 not being related to the test failures.
Looking at the history of streamplot.py and its test file, i make the following observations:
- The test cases have been renamed bd63121. I will do the same with mine, to comply to the others.
- My observation about the different starting points seem to be related to commit 71b9ae8. Following this a new test case has been submitted 20959b3, which uses the starting_points option, which is also used by my tests. I therefore conclude, that this fix results in the changes of my test and will resubmit the expected image (Since it otherwise seems to be fine).
Can you rebase and remove the extra copies of the test images?
Do an interactive rebase, move the update commits right below the original addition of the files, and change the update commits to 'fixup'.
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.
Some minor stuff + the rebase mentioned earlier.
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.
visibilty -> visibility
Also, wrap at 80.
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.
Unrelated change?
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.
Yes, but this is generated by boilerplate.py. Maybe it was changed in another commit, but boilerplate.py was not run? I rerun boilerplate.py in the 9634c0e.
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.
I think it is because pyplot was not updated with f1b89b2 change
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.
That does seem right. Should this be fixed in another issue?
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's fine to update this here. We should be better about not letting pyplot get 'stale', but it is what it is.
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.
Okay. I guess this request can therefore be closed.
I rebase again yesterday. Again there are unrelated changes from boilerplate.py (4775d7e
).
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.
foward -> forward
- space after commas
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.
Misaligned.
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.
Why not place it all in one string from the onset?
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.
Indent should be a multiple of 4.
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.
Add space after commas in above 3 lines.
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.
Add space after commas in above two lines.
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.
Space after comma.
# Das ist die erste Commit-Beschreibung: Test streamplot maxlength feature. # Commit-Beschreibung matplotlib#2 wird ausgelassen: # Added streamline maxlength test png baseline image.
@QuLogic Github still shows me a requested change, but i think i did all revisions? Otherwise please leave me a hint what is missing. Thanks!
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.
Can you please remove the "*". They are not helpful for readability.
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.
Of course i could, but all parameters in this function use the "*". It seems inconsistent if i only remove them for the new "maxlength" parameter?
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.
Can you specify the default in the docstring?
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.
Oh, according to numpydoc, it should be braces {} instead of brackets [], then the first value is the default (so put 'both' first.)
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.
I don't think this needs to be public. Can you make this function private?
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.
Ok, if it was public, i should leave it like that?
Hi @gzahl
It looks good overall, but do you mind making that new function private? Else, it adds this to the list of elements that need to be backward compatible :)
The "*" removal is just bonus points: we can merge this PR without that nitpick being fixed.
Unfortunately, that function is not new (WRT this PR, at least.)
Indeed, it is not... (I am clearly not caffeinated enough).
Glad this finally got merged!
@gzahl Thanks for your contribution and sorry the review took so long!
Thanks @tacaswell and all the others for your patience!
Hello,
I have added the option to specify the maximum streamline length. This is handy if one e.g. only needs a single (or a few) streamlines. It is especially handy if used together with "start_points".
This may be enhanced by forcing only forward (or backward) streamline integration. But that should probably be taken care of in another patch. Do you agree?