[TST] Make jpl units instantiated with datetimes consistent with mpl converters

[ksunden]

PR summary

This is a minimal change to make the usage from our tests consistent with expectations.

As far as I can tell, the inconsistency for datetimes actually predates the switch of mpl epoch from Jan 0, 0000 to Jan 1, 1970.
We only ever actually instantiate with datetime objects in tests (or via adding/subtracting durations, which don't actually care about the epoch).
I suspect that usage of jpl_units.Epoch that arent instantiated datetimes are actually incorrect due to the mpl epoch change.
They are accidentally measuring from mpl epoch due to usage of date2num and num2date, But they think they are measuring form Jan 1, 0000 (1 day off from what the code assumes is mpl's epoch).
Thus in effect, they are actually measuring from 1969-12-31 when run through the EpochConverter.

This change does not affect any of that usage, only correcting the offset when passed as a datetime object.

The change to EpochConverter is simply getting rid of the off by 1 from the outdated mpl epoch.
The changes to tests were done to make the output image unchanged but consistent with the date ranges in code.

@TD22057, do you have any opinions on these changes (or the inconsistencies that aren't addressed by this diff)?

A minimal example of the inconsistency:

>>> from matplotlib.testing import jpl_units
>>> import datetime
>>> dt = datetime.datetime(2023, 5, 4)
>>> mpl.dates.DateConverter().convert(dt, None, None)
19481.0
>>> ep = jpl_units.Epoch("ET", dt=dt)
>>> jpl_units.EpochConverter().convert(ep, None, None)
19482.0  # Off by 1 for the same input datetime!!

PR checklist

• "closes #0000" is in the body of the PR description to link the related issue
• new and changed code is tested
• Plotting related features are demonstrated in an example
• New Features and API Changes are noted with a directive and release note
• Documentation complies with general and docstring guidelines

[ksunden]

There was at least one other test that used the jpl Epoch objects, but the dates were not actually in the image generated (only times were), so the image test did not actually fail.

[tacaswell]

@TD22057 Can you confirm that this will not affect you?

[efiring]

I think we should go ahead with this. Maybe check to see whether someone has replaced Ted Drain, and can therefore look at it; but we can't hold things like this up forever.

[TD22057]

Sorry - this got lost in personal github traffic (vs work github traffic). Looks fine to me. We've kept our production converters up to date as changes have been made so we're good.

[efiring]

@ksunden I'm guessing the test failures are unrelated, correct?
@jklymak, second review and merge if OK?

[efiring]

On second thought,...
Although this fixes an internal inconsistency, it leaves in place untested behavior that is not consistent with matplotlib's current default epoch. If the Epoch object is initialized with matplotlib's datenum, as described in the Epoch.__init__ docstring,

       or using a matplotlib day number
        #   Epoch('ET', daynum=730119.5)

it will be incorrect. If no one is using this or looking at it, no harm is done, but it's a bit ugly to leave it this way.
I think that changing the docstring example above, plus changing all instances of 1721425.5 to 2440587.5 throughout the subpackage would make it fully consistent with mpl's epoch. (untested; the suggested jd offset is from https://aa.usno.navy.mil/calculated/juliandate?ID=AA&date=1970-01-01&era=AD&time=00%3A00%3A00.000&submit=Get+Date)

[jklymak]

I don't see why these two floats need to be made consistent? Some people (jpl, apparently) think that noon, Jan 1 1970 is equal to 1.5. Others, (Matplotlib, apparently) think noon Jan 1 1970 is equal to 0.5.

But I don't see what the two floats have to do with each other - they could have arbitrary offsets, and it shouldn't matter, right? Of course our locators and formatters won't work, but they obviously never worked before, it's just now they are off by one day rather than 1970 years.

[TD22057]

FYI - everything time related is generatlly arbitrary. It's not accurate to say we "think noon is equal to 1.5" - Julian date is a defined format used by astronomers that is Julian days past 4713 BC with the x.0 value at noon (so a night of observations occur in a single integer date range). In astro software it's very common way to express dates as seconds past a fixed Julian epoch - so seconds past noon of that date because it allows for accurate numeric counts of seconds to be translated back to calendar dates. The point of the unit classes was to allow arbitrary representations (\anything you want) to be converted back to what MPL needs to work properly.

The JPL units class was written as a prototype for the original unit conversion software (which we paid John Hunter to add) to show what kinds of operations needed to be supported. We don't actually use any of these classes - they were stand alone prototypes to help with defining the original units implementation. At this point the converters have evolved a lot.

IMO the main reason to keep these around (or at least something like them) is to expand unit tests to show that any function than can accept units is being tested with user defined unitized data. Over the years there have been cases where our code breaks because of a change in MPL which assumed an input was a float or numpy array instead of an arbitrary unit object which unit tests like that would help with. We haven't had time to help much lately but in the long run we'd like to contribute more test cases like that. Whether that means these tests should be kept right now or not is up to you all - I'm not sure they add a lot of value at this point but haven't looked carefully enough to know for sure.

[jklymak]

Thanks @TD22057. My phrasing was a little bit tounge-in-cheek - a constant aggravation in published literature is people not defining which yearday convention they are using and people subsequently being confused.

However, I think we agree that these offsets are arbitrary, and my point here is why we are bothering to make the jpl_units conversion agree with the Matplotlib one?

My understanding is the same as yours - we keep the jpl_units tests around so that we can test that the style of interface keeps working. We have been bad about testing all the methods versus the mock jpl_units interface, but we have a push on now to at least test our native datetime interface. #26864. I'm not certain that our native interface is similar enough to jpl_units to count as testing both.

[ksunden]

Yes, the CI errors were just the Tk version problems we had(/have, but sidestep) with azure.

I was only doing the minimal change here to make the usage in the tests consistent with expectations.

I don't have an issue with updating the float to make other usage consistent, but we don't actually use those code paths, so that is less important to me, I was just surprised when I compared the code to the test images and saw an off by one, so I wanted to at least get that far.

I don't think datetime is sufficient, but perhaps the combination of datetime and the other internal Quantity type (defined here could be enough, as that also tests the unit registry a bit more fully.

[jklymak]

I'm still confused - is the issue that test_axvspan_epoch ends up using our locators and formatters?

[ksunden]

The main issue is that Epoch(datetime(2023, 5, 4)) when plotted shows up plotted one day latter (2023-05-05).

The mechanism of this has to do with the internal representation of epochs plus using:

• date2num which provides a number that mpl formatters/locators expect
• offsetting by 1721425.5 to get to the internal representation
• offsetting by 1721425.5 + 1 to give the number to locators/formatters

It is the fact that those two offsets are different that is the problem.
This PR fixes it by adjusting the one in the converter to match the one used in the unit ingestion.

Whether it should be offsetting a different offset in the unit class itself or not is a separate question, that calls into question whether creating Epoch objects via mechanisms other than datetime are working as intended, but that is not exercised in our test suite, so is to some degree a moot point as the purpose of this class is purely for our test suite.

[ksunden]

From what I can tell, the intent is that the EpochConverter should be compatible with mpl.dates formatters/locators, it just gets the the conversions slightly wrong. There are no formatters/converters provided by the jpl_units package, and I don't think the intent would be to locate/format using non-date aware locaters/formatters. The EpochConverter rather explicitly sets the AxisInfo using mpl.dates.AutoDate[Locator|Formatter]

[jklymak]

I think this is right, and this probably hasn't worked for quite a while.

Note that the underlying Epoch object now returns incorrect Julian dates, if anyone wants those, but maybe we don't care too much