DEPR: emit a FutureWarning when mutating Time.location post-initialization

[neutrinoceros]

Description

Fixes #16061

• By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

[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 instructions for rebase and squash.
• 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?
• 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.

[github-actions]

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

[mhvk]

This looks good! Only two small suggestions.

However, I think this is also an API change - given that we explicitly set location in the tests, it is quite possible other people do that in their code too, so may be good to have the warning.

[mhvk]

Could define _location = None on the Time class

[neutrinoceros]

This works, but I have a personal (slight) aversion for using class attributes as defaults for instance attributes, because I've been bitten by some nasty bugs that would have been easier to figure out if things were kept simpler.
I don't think this matters too much here, so let @taldcroft be the judge, and I'll go with whichever version he prefers :)

[taldcroft]

@neutrinoceros - I suggested something different below, but in general I like class attribute as defaults since they more explicitly highlight the attributes of an object. This is pretty much the entire basis for dataclasses, which I've started using in new code. By using only the defined class attributes the code feels more robust. The days of tossing new attributes onto an object anywhere along the way are gone. 😄

[neutrinoceros]

I like dataclasses but they feel entirely different to me (small and self-contained VS large classes + inheritance trees). In general, the issue (to me) is in figuring out where attributes come from. Anyway, we digress ! I like your suggestion best, so let's try that !

[mhvk]

Can you also add a test where the time is initialized with a location and one tries to change it afterwards?

[neutrinoceros]

I've parametrised the test to this effect !

[mhvk]

I think this counts as an API change, so changed the label/milestone accordingly. Though perhaps good to see what @taldcroft thinks... It certainly was not the intent to be able to modify location...

[taldcroft]

I'm fine with making location immutable.

[mhvk]

I'm fine with making location immutable.

@taldcroft - the question I had is whether you think that change can be backported (I felt it couldn't, but could go either way).

[neutrinoceros]

I must have forgotten to undraft this. In any case, thank you @mhvk for your early review !

[taldcroft]

@neutrinoceros - definitely this cannot be backported. Having seen that the astropy fits tests rely on location mutability makes me worry about this breaking user code. I didn't think about the implications enough when I earlier said that making location immutable is fine.

Although it doesn't impact the original hash id issue, if the location is an array then even with this code it is still mutable. That would imply in this case we need to make the location array be read-only. I also wonder if this changing the location can result in a bug due to caching of format values. Might this be a problem @mhvk?

Maybe one more gentle path forward is to make changing the location in the scalar case raise a future deprecation warning. This might be something like below. This should also allow reverting most or all of the changes in time/core.py. It lets you set .location the first time if it has never been accessed.

    @property
    def location(self) -> EarthLocation | None:
        if not hasattr(self, "_location"):
            self._location = None
        return self._location

    @location.setter
    def location(self, value):
        if hasattr(self, "_location"):
            warnings.warn(
                "In a future version the location attribute will be immutable once set.
                "Instead you should set the location when creating the Time object.",
                FutureWarning,
                stacklevel=2,
            )
        self._location = value

[neutrinoceros]

if the location is an array then even with this code it is still mutable. That would imply in this case we need to make the location array be read-only

wait. How flexible is this attribute's type anyway ?

Maybe one more gentle path forward is to make changing the location in the scalar case raise a future deprecation warning.

It's less agressive to users so I like it, I'll try this now.

[neutrinoceros]

This should also allow reverting most or all of the changes in time/core.py. It lets you set .location the first time if it has never been accessed.

While integrating your suggestion, I found that making the warning go off only the first time the attribute is mutated required an additional layer of complexity that I don't think is warranted: the default warning filters should be enough to protect users against high noise levels, plus if we really want to be gentle about this API change, we might as well keep allowing arbitrarily numerous mutations for now. So, I'm keeping it simple for now. As a result, I also didn't revert my other changes: internal mutations are still considered allowed and should never trigger the warning, so it's probably simpler in the current state.

[mhvk]

It looks good to me! I'm never quite sure what warning to use - should this just be AstropyDeprecationWarning?

[neutrinoceros]

Experts can come in an correct me, but my rule of thumb here is that (Astropy)DeprecationWarnings are for entire functions, classes and arguments that won't be available in the future, and FutureWarnings are for other API changes we plan to make later. There's also that saying (I forget the source) that DeprecationWarnings are for devs and FutureWarnings are for end-users, and this felt like something that would primarily affect end-users.

[taldcroft]

Looks good, thanks @neutrinoceros !

[pllim]

AFAIK, we never (or at least seldom) use FutureWarning for deprecations in this package. I wanted to make a follow-up issue but I don't know what to follow-up on here. Do we turn this into AstropyDeprecationWarning in the future, or this is considered deprecated already because you are emitting some warning and we just remove this stuff after 2 releases? Please advise. Thanks!