Axis should always honor absolute minimum /maximum as well as minimum/maximum range properties#1819
Conversation
|
Thanks for putting this together! I'm pretty busy for the next few days, but will try to take a proper look at Thursday. Feel free to ping me if I don't get back to you or if you have any questions. |
VisualMelon
left a comment
VisualMelon
left a comment
There was a problem hiding this comment.
I had some reservations about the behaviour around NaNs here, but I think that's an edge case we probably don't need to worry about for now given how limited the scenarios are where this behaviour would be applicable.
(e.g. the implicit NaN check on line 1602 in Axis isn't very obvious; might be worth making that and other implicit checks more explicit)
|
@Jonarw would you mind sanity checking this? I'm always nervous about merging this sort of change without a second review. |
|
@mghie that last commit has gone wrong, I think: are you able to revert and rebase against the develop branch? |
|
@VisualMelon I did a revert and rebase (first time I did such a thing, hopefully the right way), does it look better now? I have to say that I'm not really sure what I'm doing here, learning along the way, please be patient... |
|
No problem, I should have been clearer. What you have looks fine when I look at it locally, but I'm not sure what is going on with the github diff interface. Instead of that, can you do a hard reset to commit f3fab4a (may want to make a backup first) and force-push that over the top? That should remove the new commits, and just leave you with the 4 proper commits. You can achieve this by running: Having done this locally, there will be a conflict with the changelog, but we (the maintainers) should be able to fix that when we merge it. Rebasing would enable you to resolve this, but it's trivial so shouldn't be a problem: if there is a problem, we can deal with that when we get there. |
An axis with absolute limits and MinimumRange set will not always honor these after a call to Reset(). Two new tests added which demonstrate the isse, one without data and the other with data partially out-of-range.
The exception message for the check that MinimumRange is not larger than the difference of absolute maximum and absolute minimum was wrong. Added also a check for MinimumRange not being larger than MaximumRange
Axis constraints like absolute minimum, absolute maximum, minimum range and maximum range should now work even with no data or data which is outside the absolute limits. The validity checks have been moved to the start of the method so that there is no partial setting of properties.
|
Thanks for rebasing as well! |
|
@VisualMelon Thanks for your support, I learned a few things today |
3 participants
Fixes #1812.
Checklist
Changes proposed in this pull request:
Axis.CoerceActualMaxMin()ActualMaximumandActualMinimumimproved to always honor absolute minimum and maximum as well as minimum and maximum range@oxyplot/admins