-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix plotting in Plots > v.1.11 #165
base: master
Are you sure you want to change the base?
Conversation
On April 20, when @AlexRobson created this PR I would have said yes. Now I'd say no. Plots v1.11 has been out since March 2021 and plots is now on v1.29.1. I think it is okay to drop support for 15 month old Plots. If a project has Plots pinned to before 1.11, then they will end up having Intervals pinned to 1.7.1 (or whenever this merges) which seems perfectly acceptable to me. |
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.
Lets not worry about supporting Plots pre-1.11
we can't directly restrict that.
we can restrict to RecipeBase 1.0, or even 1.2, which is the minimum for Plots v1.24
We should drop support for RecipesBase 0.8 and 0.9
so lets make the changes to the Project.toml, including bumping the version number
and release this yes?
I assume this is manually tested, and i think automatically testing this more than we currently are isn't work the effort.
Though perhaps the current automtic tests needs updating?
I am not sure what is up with them since they expired
I'm pikcing this back up. Just working through it now. I should have an update shortly. |
Looking at this again, I recall the reason I hit a block on it, and why it shouldn't go in as-is. There are two reasons for this, one major and one minor. The major reason is that this seems to break with DateTime Intervals. As-is, this will throw an error in one of the tests that include datetime intervals:
However, I suspect that this may need to be handled in a different package, I'm playing around with the other packages in the stacktrace to track it down (e.g. RecipesPipelines). The minor reason is the change in behaviour means that the intervals all get marked as a different series. For non-datetime intervals:
|
This is also signficiant since if we had multiple series (like we often do) we won't be able to tell them apparent and the legend will be useless |
This may be helpful: if you set |
Thanks - this fixed that one :) - the last thing for me in this MR then is tracking down why DateTime isn't working. |
One thing that may be worth keeping in mind is that until more recent versions of Plots (improved in 1.23.x, fully fixed by 1.30.2), adding many series could get extremely slow because there were several O(n^2) operations over series. I discovered and fixed them, but depending on how many intervals you expect people to be plotting, and if people are on an older version, they could have a negative experience there. >100 or >1000 is where it might get really annoying. |
fe45b0f
to
bb6d24e
Compare
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
==========================================
- Coverage 84.16% 83.74% -0.43%
==========================================
Files 12 12
Lines 840 818 -22
==========================================
- Hits 707 685 -22
Misses 133 133
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Fixes ##159
Plots changes the way it handles line segments. Instead of
NaN
separated, it uses a vector of points (see issue).Should we keep support for Plots < v1.11