Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Feb 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 17 additions & 8 deletions astropy/io/fits/tests/test_fitstime.py
Expand Up @@ -42,11 +42,14 @@ def test_time_to_fits_loc(self, table_types):
columns in a ``Table``.
"""
t = table_types()
t["a"] = Time(self.time, format="isot", scale="utc")
t["b"] = Time(self.time, format="isot", scale="tt")

# Check that vectorized location is stored using Green Bank convention
t["a"].location = EarthLocation([1.0, 2.0], [2.0, 3.0], [3.0, 4.0], unit="Mm")
t["a"] = Time(
self.time,
format="isot",
scale="utc",
location=EarthLocation([1.0, 2.0], [2.0, 3.0], [3.0, 4.0], unit="Mm"),
)
t["b"] = Time(self.time, format="isot", scale="tt")

with pytest.warns(
AstropyUserWarning,
Expand Down Expand Up @@ -83,15 +86,19 @@ def test_time_to_fits_loc(self, table_types):
assert tm["b"].location == t["b"].location

# Check that multiple Time columns with different locations raise an exception
t["a"].location = EarthLocation(1, 2, 3)
t["b"].location = EarthLocation(2, 3, 4)
t["a"] = Time(
self.time, format="isot", scale="utc", location=EarthLocation(1, 2, 3)
)
t["b"] = Time(
self.time, format="isot", scale="tt", location=EarthLocation(2, 3, 4)
)

with pytest.raises(ValueError) as err:
table, hdr = time_to_fits(t)
assert "Multiple Time Columns with different geocentric" in str(err.value)

# Check that Time column with no location specified will assume global location
t["b"].location = None
t["b"] = Time(self.time, format="isot", scale="tt", location=None)

with pytest.warns(
AstropyUserWarning,
Expand All @@ -104,7 +111,9 @@ def test_time_to_fits_loc(self, table_types):
assert len(w) == 1

# Check that multiple Time columns with same location can be written
t["b"].location = EarthLocation(1, 2, 3)
t["b"] = Time(
self.time, format="isot", scale="tt", location=EarthLocation(1, 2, 3)
)

table, hdr = time_to_fits(t)

Expand Down
43 changes: 32 additions & 11 deletions astropy/time/core.py
Expand Up @@ -5,6 +5,7 @@
UT1) and time representations (e.g. JD, MJD, ISO 8601) that are used in
astronomy.
"""
from __future__ import annotations

import copy
import enum
Expand All @@ -14,6 +15,7 @@
from collections import defaultdict
from datetime import date, datetime, timezone
from time import strftime
from typing import TYPE_CHECKING
from warnings import warn
from weakref import WeakValueDictionary

Expand Down Expand Up @@ -46,6 +48,8 @@
from .time_helper.function_helpers import CUSTOM_FUNCTIONS, UNSUPPORTED_FUNCTIONS
from .utils import day_frac

if TYPE_CHECKING:
from astropy.coordinates import EarthLocation
__all__ = [
"TimeBase",
"Time",
Expand Down Expand Up @@ -554,7 +558,7 @@
# collected by the TimeAstropyTime format class up to the Time level.
# TODO: find a nicer way.
if hasattr(self._time, "_location"):
self.location = self._time._location
self._location = self._time._location

Check warning on line 561 in astropy/time/core.py

View check run for this annotation

Codecov / codecov/patch

astropy/time/core.py#L561

Added line #L561 was not covered by tests
del self._time._location

# If any inputs were masked then masked jd2 accordingly. From above
Expand Down Expand Up @@ -744,6 +748,24 @@

raise TypeError(f"unhashable type: '{self.__class__.__name__}' {reason}")

@property
def location(self) -> EarthLocation | None:
return self._location

@location.setter
def location(self, value):
if hasattr(self, "_location"):

Check warning on line 757 in astropy/time/core.py

View check run for this annotation

Codecov / codecov/patch

astropy/time/core.py#L757

Added line #L757 was not covered by tests
# since astropy 6.1.0
warn(

Check warning on line 759 in astropy/time/core.py

View check run for this annotation

Codecov / codecov/patch

astropy/time/core.py#L759

Added line #L759 was not covered by tests
"Setting the location attribute post initialization will be "
"disallowed in a future version of Astropy. "
"Instead you should set the location when creating the Time object. "
"In the future, this will raise an AttributeError.",
category=FutureWarning,
stacklevel=2,
)
self._location = value

Check warning on line 767 in astropy/time/core.py

View check run for this annotation

Codecov / codecov/patch

astropy/time/core.py#L767

Added line #L767 was not covered by tests

@property
def scale(self):
"""Time scale."""
Expand Down Expand Up @@ -1377,7 +1399,7 @@
)

# Optional ndarray attributes.
for attr in ("_delta_ut1_utc", "_delta_tdb_tt", "location"):
for attr in ("_delta_ut1_utc", "_delta_tdb_tt", "_location"):
try:
val = getattr(self, attr)
except AttributeError:
Expand Down Expand Up @@ -1953,14 +1975,13 @@
from astropy.coordinates import EarthLocation

if isinstance(location, EarthLocation):
self.location = location
self._location = location
else:
self.location = EarthLocation(*location)
if self.location.size == 1:
self.location = self.location.squeeze()
else:
if not hasattr(self, "location"):
self.location = None
self._location = EarthLocation(*location)

Check warning on line 1980 in astropy/time/core.py

View check run for this annotation

Codecov / codecov/patch

astropy/time/core.py#L1980

Added line #L1980 was not covered by tests
if self._location.size == 1:
self._location = self._location.squeeze()
elif not hasattr(self, "_location"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could define _location = None on the Time class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

self._location = None

if isinstance(val, Time):
# Update _time formatting parameters if explicitly specified
Expand All @@ -1984,7 +2005,7 @@
):
try:
# check the location can be broadcast to self's shape.
self.location = np.broadcast_to(self.location, self.shape, subok=True)
self._location = np.broadcast_to(self._location, self.shape, subok=True)

Check warning on line 2008 in astropy/time/core.py

View check run for this annotation

Codecov / codecov/patch

astropy/time/core.py#L2008

Added line #L2008 was not covered by tests
except Exception as err:
raise ValueError(
f"The location with shape {self.location.shape} cannot be "
Expand Down Expand Up @@ -2777,7 +2798,7 @@

location = self.location[tuple(sl)]

result.location = location
result._location = location

Check warning on line 2801 in astropy/time/core.py

View check run for this annotation

Codecov / codecov/patch

astropy/time/core.py#L2801

Added line #L2801 was not covered by tests
return result

def __array_function__(self, function, types, args, kwargs):
Expand Down
13 changes: 13 additions & 0 deletions astropy/time/tests/test_basic.py
Expand Up @@ -2883,3 +2883,16 @@ def test_timedelta_empty_quantity():

with pytest.raises(ValueError, match="only quantities with time units"):
TimeDelta([] * u.m)


@pytest.mark.parametrize(
"kwargs", [{}, dict(location=None), dict(location=EarthLocation(0, 0, 0))]
)
def test_immutable_location(kwargs):
# see https://github.com/astropy/astropy/issues/16061
loc = EarthLocation(0, 0, 0)
t = Time("2024-02-19", **kwargs)

with pytest.warns(FutureWarning):
# in the future, this should be an AttributeError
t.location = loc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've parametrised the test to this effect !

1 change: 1 addition & 0 deletions docs/changes/time/16063.api.rst
@@ -0,0 +1 @@
A ``FutureWarning`` is now emitted when mutating ``Time.location`` post-initialization.