dates.YearLocator doesn't handle inverted axes

[elpres]

All other Locators are derived from RRuleLocator, which has the following check inside its __call__ routine:

try:
    dmin, dmax = self.viewlim_to_dt()
except ValueError:
    return []

if dmin > dmax:
    dmax, dmin = dmin, dmax

YearLocator just gets values for dmin and dmax in the same manner as above and goes on using them without checking for ordering and swapping if necessary. This leads to only one tick on inverted axes using YearLocator.

[tacaswell]

cc @cimarronm This looks like #3522 / #3524

[cimarronm]

It seems to me that ideally YearLocator should be modified to use RRuleLocator to be consistent with others (RRuleLocator automatically handles the inverted axis) but there may be a reason why it cannot. I'll look into it more tonight

[pelson]

I'm comfortable with this change - it needs a test (which should not be a visible test)

It seems to me that ideally YearLocator should be modified to use RRuleLocator to be consistent with others (RRuleLocator automatically handles the inverted axis) but there may be a reason why it cannot. I'll look into it more tonight

That is maybe a nice to have, but is by no means necessary IMHO. Let's get this in, and then any refactoring can be done as a separate activity - the good news is that there will already be a test which exercises the desired behaviour 😉

[cimarronm]

I'm in agreement with @pelson. Let's get this in along with a test and we can look at possibly updating in a separate PR

[tacaswell]

The revision needed is a test.

[elpres]

I've noticed that there is another method of YearLocator (autoscale) which contains the same kind of pattern, and added an identical check to that as well. However, seeing that in all cases when the min-max pairs are recieved in YearLocator and RRuleLocator, they come from the methods DateLocator.datalim_to_dt and DateLocator.viewlim_to_dt, maybe it more sensible and elegant to include the ordering check there and remove it from the consumers. But I don't know the code well enough to make the call here and leave the decision to you guys.

[elpres]

In the previous two commits I've moved the fix to the parent class, as described before. This also makes AutoDateLocator work in the same situation, where it previously aborted with

ValueError: No sensible date limit could be found in the AutoDateLocator.

[tacaswell]

This looks reasonable to me, but it still needs a test.

[cimarronm]

I'm not sure I agree with the latest changes. It seems like you are breaking API by changing the return value of the data and view limits instead of returning what was set. I think your original method for handling it inside YearLocator was the correct way.

Note that the equivalent type of thing is done in axis e.g. axis._update_ticks to handle the scenario where the order is reversed rather than just modifying get_data_interval from returning it already in min, max order and, of course, already by RRuleLocator.

[elpres]

Yes, I agree that it's an API change, but the question is whether anything will notice it. From what I can tell, there are three classes on the receiving end of this change:

• RRuleLocator test the values and reverses if necessary. The code after that point expects dmin to be <= dmax.
• YearLocator also expects dmin and dmax to be ordered, but it doesn't check, so it breaks.
• same for AutoDateLocator.

That means all of them expect the correct ordering, and all of them have to check and swap the values they receive. So, from their point of view, the return values of datalim_to_dt and viewlim_to_dt are in an undefined state which they each have to fix manually in the same way. Continuing to return the two values in the same order as now means that whenever somebody subclasses DateLocator in the future, they will have to be aware of the situation and remember to put this check in there too. Sorting the limit in the parent seems to me to be 1) more robust, and 2) safe.

[tacaswell]

punting to 1.4.x as this still needs work and has been silent for 2 months.

I now agree with @cimarronm that we should not change the API, but can be convinced otherwise. If it does get changed this will need to documented in https://github.com/matplotlib/matplotlib/tree/master/doc/api/api_changes and have docstrings added to the methods which make clear they are doing the order rectification.

This still needs a test.

[elpres]

The API in this case doesn't seem to have been clearly defined in the first place, since the existing Locators deal with the two possible orientations of axes differently (some do check for direction, some don't). The change I've proposed removes ambiguity and code duplication, which is IMHO an improvement. It also resolves the problem at the root instead of in the branches, which I would also consider the better way. And as far as I can say, there should be not breakage coming from that.

As for a test, in a simple form it would have to look something like this:

1. Create an isolated Axis object.
2. Set its locator to YearLocator and the range so that it results in a reasonable amount of labels.
3. Store the labels.
4. Invert the axis.
5. Make sure the labels are the same but in reversed order.
6. Repeat for AutoDateLocator.

But I don't know the low level API enough (or at all) to know how to e.g. instantiate an Axis and make it behave the way it would when it's a part of a chart. Also, I don't know your conventions for writing tests. So I'm sorry, but at the moment I don't have the spare energy to invest into learning these things and would rather leave this to somebody with more experience.

[tacaswell]

@elpres This needs a rebase

[elpres]

@tacaswell This was the first time I've ever done a git rebase (I use mercurial for my own projects, and never had to do any sort of modification of this complexity), and looking at the log above I likely messed something up. If I really did, I can open a new, clean pull request, as the patch itself is rather easy to re-create.

[tacaswell]

Something has gone very wrong with this rebase, it should have 2 commits, not 200+ 😉

See #2742 for some docs on how to do this right (that PR was stalled on what to call the example remotes).

[tacaswell]

Don't make a new PR, just force-push to this branch when you clean it up and gh will 'do the right thing'

[elpres]

Ok, this time it seems to have worked out properly (right?). Thank you for linking the rebase howto; it was very helpful.

[WeatherGod]

still doesn't have a unit test. Don't want to regress this feature in the future.

[tacaswell]

Giving up on the unit tests for now.