Disallow the use of quantity input for 'decimalyear' format.

[mhvk]

Using quantities with units of time for Time format 'decimalyear' will now raise an error instead of converting the quantity to days and then interpreting the value as years. An error is raised instead of attempting to interpret the unit as years, since the interpretation is ambiguous: in 'decimaltime' years are equal to 365 or 366 days, while for regular time units the year is defined as 365.25 days.

Fixes #14541

[github-actions]

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[mhvk]

dev failures unrelated (URL timeouts)

[pllim]

https://datacenter.iers.org appears to be down. Do you know who we should contact?

[mhvk]

I think it is temporary - I can open the URL now.

[pllim]

I restarted the remote data job

[pllim]

There are two real failures on the remote data job (that also picks up numpy dev), so I cannot tell if they are related to your PR or not: https://github.com/astropy/astropy/actions/runs/4495274221/jobs/7921203286?pr=14566

[mhvk]

Both failures are weird, but definitely unrelated to this PR. It looks like a location has moved according to google (I guess we just pick a more guaranteed example...), while something much have changed with np.fix. Will investigate...

[taldcroft]

Looking at the code it looks like there is another related mistake that the jyear (TimeJulianEpoch) format does not check for a Quantity input. It seems like the _check_val_type(self, val1, val2): method in TimeBesselianEpoch should instead be in TimeEpochDate.

But with 3 classes using the same checker maybe it is time for a mixin class that provides just that _check_val_type method?

[mhvk]

@taldcroft - jyear can work with Quantity, because it has the same definition of a year (of 365.25 days), so I think it is just byear and decimalyear.

In [3]: Time(365.25*2000*u.day, format='jyear')
Out[3]: <Time object: scale='tt' format='jyear' value=2000.0>

But I'm happy to combine byear and decimalyear. What do you think about the comment of @eerovaher that it should be a TypeError instead of a ValueError?

[taldcroft]

jyear can work with Quantity, because it has the same definition of a year (of 365.25 days), so I think it is just byear and decimalyear.

Personally I find this surprising, or at least contrary to the documentation for the float TimeEpoch classes. E.g. """Julian Epoch year as floating point value(s) like 2000.0.""". For me, all the Time Format classes are representations of a point in time and cannot be set with a Quantity which is an interval of time. I would consistently apply that even for time formats that are defined as an interval of time since an epoch.

Alas that ship has already sailed and some of the epoch classes will accept a Quantity and some won't, so I agree with the approach here to fix DecimalYear. For extra credit you might expand the PR scope a bit and update the doc strings to indicate which ones also accept a Quantity.

[mhvk]

Yes, perhaps in hindsight it was not that great an idea to allow quantities in Time instead of just in TimeDelta. But I think it makes a bit more sense when you consider both val and val2 since they can have different units, and one can think of val2 as a TimeDelta that gets added to val. Anyway, as you say, that ship has sailed; we just have to make sure that we refuse the temptation to guess (which is easy for jd, mjd, and jyear, etc.)

[mhvk]

@taldcroft - I added an extra commit that refactors out a new TimeNumericNoQuantity class and uses that for both byear and decimalyear. I also adjusted a few docstrings, but did not make a note for all. I considered editing the time documentation (perhaps the description of val) but then realized that this part also mentions that scale is a required parameter, so it would be more of an update than I have time for right now...

[mhvk]

Actually, a failure makes clear that we do have a bit of text in the Time documentation: https://docs.astropy.org/en/latest/time/index.html#interaction-with-time-like-quantities

[mhvk]

OK, tests all passed, ready for final review...

[mhvk]

Thinking about this again, I'm not 100% sure I like the new TimeNumericNoQuantity class as it introduces API that I'm not sure we want to support forever. Shall I just revert that part, duplicating the check? (But keeping the docstring updates.)

[taldcroft]

@mhvk - one possibility is a helper function or static method that implements the no-Quantity check. Lately I find myself trying to avoid so much inheritance. An advantage of helper functions is that they can be easier to test in unit tests, and it is definitely easier to understand when reading the code. E.g.

class TimeDecimalYear(TimeNumeric):
    def _check_val_type(self, val1, val2):
        _check_val_type_not_quantity(self.name, val1, val2)  # check val1 and val2 are not Quantity
        return super()._check_val_type(val1, val2)

[mhvk]

OK, I went with your suggestion - that does leave us free to change things if we find a better way to do this. (E.g., I experimented with unit = None, which worked except it is also used to determine the number of digits in string output. Which is probably better done another time!

[taldcroft]

@mhvk - looks great! CI hasn't fully passed yet, but assuming that it does you can click the "CI passed" checkbox and merge.