Skip to content
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

[WIP] Steppath and Line2D #1876

Closed
wants to merge 4 commits into from
Closed

[WIP] Steppath and Line2D #1876

wants to merge 4 commits into from

Conversation

Tillsten
Copy link
Contributor

@Tillsten Tillsten commented Apr 1, 2013

Ok, this is a first try to refactor Lines2D for steps. Submitted as a PR to run the tests. See #1709.

@Tillsten Tillsten closed this Apr 1, 2013
@Tillsten Tillsten reopened this Apr 1, 2013
@Tillsten
Copy link
Contributor Author

Tillsten commented Apr 1, 2013

So tests are still passing, any other concerns with this approach?

@Tillsten
Copy link
Contributor Author

Ok, because i not sure if it is worth to continue this PR i am closing this.
I am still thinking that refactoring out the path calculation is the right think to do, but i am not so sure about the API change. Maybe just adding a get_drawn_path() is a better way.

@Tillsten Tillsten closed this May 29, 2013
@mdboom
Copy link
Member

mdboom commented May 29, 2013

@Tillsten: Other than the lack of response (for which I apologise -- the PR queue can be a real firehose sometimes), what's your reason for closing this? It seems reasonable...

@Tillsten
Copy link
Contributor Author

@mdboom Mostly because i not sure if this is the right way to do it: the idea of the old design seems to be that the difference between draw_styles is only relevant for drawing.

The biggest practical concerns i have, is that i am not sure if line.set_data still works as as expected. The fact the test-suit works seems to imply it does, but i am still unwary. The return value of get_data now returns also something different (probably more corrected now), which is the aim of the PR.

@tacaswell
Copy link
Member

I would put a vote in for not having the function change the data so that set_[x,y]data/get_[x,y]data round trip works. Aside from my sense of symmetry, changing the data would mess up animation code where you append new data onto existing lines (by using get_[x,y]data -> append -> set_[x,y]data).

@Tillsten
Copy link
Contributor Author

@tacaswell But the problem still remains, that at the moment you don't have any method to get the drawn data. Also the step-vertices are recalculated very draw.

@tacaswell
Copy link
Member

@Tillsten What do you mean by drawn data?

Is the calculation too slow to run every draw?

@Tillsten
Copy link
Contributor Author

Tillsten commented Jun 3, 2013

@tacaswell The problem that there is no way of getting the "real" vertices from a path which use steps. This problematic if you want to make a filled histogram. Speedwise: I not sure, i don't use mpl for a lot of interactive work because it is quite slow, so i didn't benchmark it. I think the difference will be only visible for a lot of step-plots.

@tacaswell
Copy link
Member

@Tillsten If you assume a bottom level, can you put in an axhline and then use fill_between?

@thriveth
Copy link

@tacaswell No, see the link in the OP here: #1709 (it is old but still applies).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants