Add warning for unitless numerical input to TimeDelta

[maxnoe]

Description

• Code like TimeDelta(5) or Time("2020-01-01") + 5 now raises an
  AstropyDeprecationWarning.
  The warning can be avoided by specifying a unit or a format explicitly:
  TimeDelta(5 * u.day), TimeDelta(5, format="jd")

Fixes #12887

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• 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.
• 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 astropy-bot check might be missing; 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.

[pep8speaks]

Hello @maxnoe 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2022-02-25 14:40:37 UTC

[taldcroft]

@maxnoe - it would be good to document the default days behavior in the context of time arithmetic in the narrative docs. Looking now at the docs I see there is plenty of room for improvement. I don't see any examples of doing arithmetic with Quantity values. It might be worth putting one example like that in Getting started.

There is a bullet point "Do time arithmetic involving Time and/or TimeDelta objects" that should also include Quantity and be backed by a sub-section with a few examples.

[maxnoe]

@taldcroft will do

[mhvk]

@maxnoe - Looks like @taldcroft and you converged to a good solution! One suggestion for more tests, one nitpick.

[maxnoe]

Now I am mightily confused, adding the example to the docs I get this when running `pytest doctest-glob="*.rst" docs/time/index.rst":

1234   # The now deprecated default assumes days for numeric inputs
1235   >>> t1 + 5.0
UNEXPECTED EXCEPTION: TypeError("unsupported operand type(s) for +: 'Time' and 'float'")
Traceback (most recent call last):
  File "/home/maxnoe/.local/anaconda/envs/astropy-dev/lib/python3.9/doctest.py", line 1334, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest index.rst[165]>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'Time' and 'float'

The same code works in python / ipython

[mhvk]

@maxnoe - I know what the issue is but am less sure how to solve it: pytest runs everything with deprecations turned into errors. But with that, it triggers the try/except at

astropy/astropy/time/core.py

Line 2364 in 179fa5f

try:

(and the similar one at

astropy/astropy/time/core.py

Line 1391 in 179fa5f

except Exception:

), so the code returns NotImplemented, which leads python to raise a TypeError. Maybe best would be to have something like

except Exception as exc:
    if isinstance(exc, Warning):  # Not sure this is right way to know it is a warning!!!
        # Warnings are treated as exceptions; just raise.
        raise exc
    else:
        # Could not create TimeDelta, so give other a chance.
        return NotImplemented

Though I'm not sure this solves the doctest issue... On the other hand, one should not ignore a warning if the user has explicitly asked for it to be treated as an exception... Which is why I'm not sure what is best...

[maxnoe]

@mhvk I think raising is correct behavior. Someone setting -Werror wants to get the exception and not the exception turned into NotImplemented.

Edit: Just turning the bare except into except Exception should fix the issue, as warning does not inherit from BaseException and it is also not wanted that KeyboardInterrupt and SystemExit are turned into NotImplemented

[maxnoe]

Now I am even more confused....

>>> isinstance(Warning, Exception)
False

But except Exception catches UserWarning with -Werror.

[eerovaher]

If you want to write an example in the documentation that emits a warning then you have to use the +SHOW_WARNINGS directive, see https://github.com/astropy/pytest-doctestplus#showing-warnings

[maxnoe]

@eerovaher Thanks, that works

[mhvk]

Looks all good, modulo an unneeded mark in the tests, which we might as well remove.

[mhvk]

Thanks, all good now!

[taldcroft]

Looks great, thanks! Just a couple of minor suggestions inline.

I'd also request that you add an example to the top Getting started section, in the example box just after "Finally, some further examples of what is possible. For details, see the API documentation below." Here can you put in an example like below. Otherwise this basic time arithmetic is buried way below the fold.

>>> Time('2020-01-01") + 5 * u.day
<Time object: scale='utc' format='iso' value=2020-01-06 00:00:00.000>

Finally, at this point it probably worth squashing to one commit.

[taldcroft]

Thanks @maxnoe! Approved with a lien on CI passing (currently in-process). I unchecked the "CI passing" box for now.

[maxnoe]

@taldcroft If I see it correctly, there is only a fail in the allowed failures section that seems unrelated.

[maxnoe]

@mhvk this also closes #10355, right?