Time value attribute incorrect when scale != epoch_scale by taldcroft · Pull Request #4312 · astropy/astropy

taldcroft · 2015-11-16T03:40:28Z

For TimeFromEpoch subclasses there is a bug with the value attribute:

In [3]: x = Time(1, format='unix', scale='tt')  # epoch_scale is UTC

In [4]: x.value
Out[4]: 41.184082029998414

In [5]: x.unix
Out[5]: 0.9999999999921627

The problem is that the comment below in TimeFormat.value doesn't apply when accessing Time.value directly (it is correct when Time.__getattr__ gets run for a format attribute):

    def value(self):
        # when we get here, getattr will already have ensured our scale
        # equals epoch scale, so we can just subtract the epoch and convert
        time_from_epoch = ((self.jd1 - self.epoch.jd1) +
                           (self.jd2 - self.epoch.jd2)) / self.unit
        return time_from_epoch

Options include patching Time.value to do the same munging that Time.__getattr__ does, or fixing the TimeFromEpoch.value property. The latter is conceptually nicer, but I think it requires that the contained TimeFormat object or TimeFormat.value() method has access to the parent Time object since scale transformations take place at that level.

It looks like it might be pretty clean to change the TimeFormat.value property into a normal method that takes an optional parent arg. Since Time._time is not public this should be OK, and the footprint of this change actually looks quite small.

The Time.__getattr__ munging definitely a hack since it requires that Time has specific knowledge about one kind of TimeFormat object:

        elif attr in self.FORMATS:
            tm = self.replicate(format=attr)
            if hasattr(self.FORMATS[attr], 'epoch_scale'):
                tm._set_scale(self.FORMATS[attr].epoch_scale)
            return tm.value

mhvk · 2015-11-16T14:20:00Z

Is there a reason not just to use getattr(parent, self.epoch_scale)? (Or are you thinking ahead to when there will be a cache?).

This is going to break if parent is None though. Meanwhile, since most of the other TimeFormat.value implementations don't even use the parent argument it seems like a lot of conceptual overhead added to support one special case. I suspect there's maybe some other way to work around this--even something like what @mhvk suggested of adding a method on the formatters that can be used internally by Time.value to do this (which would still just return self.value in most cases). But keep it an internal method.

In other words, like @mhvk suggested have some kind of TimeFormatter._to_value() method that serves the purpose that's been tacked on to TimeFormatter.value here. The one difference is I would keep it an internal method.

Is there a reason not just to use `? (Or are you thinking ahead to when there will be a cache?).

Just the converse, I'm thinking of the case right now where getattr(parent, self.epoch_scale) always makes a replica even if the scale is not changing. In the future when that is efficient then this code could be cleaned up. Since I really want this particular bug fixed for my own work (I use cxcsec), I was doing the minimal change.

This is going to break if parent is None though.

Yup, I thought about that... so perhaps it would have been more correct to just make parent a positional arg?

mhvk · 2015-11-16T14:35:22Z

@taldcroft - on the implementation, wouldn't it be cleaner to just have all the formats have a to_value method (just like TimeDateTime), leaving value as a property that points to it?

More generally, does the above now give the right answer? I'm not sure I would expect t.tt.cxcsec and t.tdb.cxcsec to mean the same thing. I'm not 100% sure I know what I would expect, but I think it would be more natural for the epoch to be transformed to .tdb. ... but maybe I'm just confused ... got to run, but will try to think a bit more later...

embray · 2015-11-16T14:35:34Z

I don't fully understand what this is about, but changing the .value attribute everywhere to a method seems like a mess of a way to fix it, and isn't this backward incompatible? (Part of my problem is I haven't been following other development, so I don't know what epoch_scale is...)

taldcroft · 2015-11-16T15:06:28Z

I felt OK changing TimeFormat.value from property to method because _time is not public. However, the idea of keeping the API for code that is cheating anyway is not a bad one. Thus the current internal calls (I think there is just one or two) to self._time.value(parent=self) would be self._time.to_value(parent=self). This won't reduce the change footprint at all but will reduce API breakage (again, on a private method).

taldcroft · 2015-11-16T15:08:22Z

On the bigger question of passing parent everywhere even though it is only needed by one class, I could not think of a way around this besides the original implementation. In the original the information about which classes are special (and what to do) is hardwired into the parent (Time) class and I don't like that.

taldcroft · 2015-11-16T15:11:41Z

I'm not sure I would expect t.tt.cxcsec and t.tdb.cxcsec to mean the same thing.

Yes, it must. cxcsec (and unix et al.) are defined as the interval of time in TAI seconds since a reference epoch. That reference epoch is not dependent on the time object scale, instead it is fixed by the class epoch_scale. For these classes the scale only has impact when you represent in a different format like ISO.

mhvk · 2015-11-16T18:38:43Z

@taldcroft - OK on the scale,

On the implementation, I think you ended up with a mix of what @embray and I recommended. By analogy with TimeDateTime, I would actually suggest to make a public to_value method which takes parent as a keyword argument (with a default value of None). This way, all format classes have the same method as TimeDatetime already has -- and these can be extended in similar ways as needed. It also means one can leave the value property in place (defined by value = property(to_value)); it would only not work in formats derived from TimeFromEpoch for the case in which the scale does not equal that of the epoch (which should just raise an exception if parent is None).

One issue is stability of the interface (even though these are within the private _time attributes). I think parent might as well be a keyword-only argument; I guess if we want to be really strict about that, we would have to define a signature to_value(self, <timezone etc>, **kwargs) and then do parent = kwargs.pop('parent', None).

mhvk · 2015-11-16T18:41:34Z

In practice, my suggestion would be

def to_value(self, parent=None): # or **kwargs, followed by parent=kwargs.pop('parent', None) if self.scale != self.epoch_scale: if parent is None: raise SomeException tm = parent.replicate() tm._set_scale(self.epoch_scale) jd1, jd2 = tm._time.jd1, tm._time.jd2 else: jd1, jd2 = self.jd1, self.jd2 time_from_epoch = ((jd1 - self.epoch.jd1) + (jd2 - self.epoch.jd2)) / self.unit return time_from_epoch value = property(to_value)

p.s. I think getattr(parent, scale) would still be fine here, since you already know that you have to change the scale, so there is no worry about loss of speed for the case that the scale is unchanged. Or am I missing something?

taldcroft · 2015-11-17T03:27:35Z

I realized I was making a big mistake of ignoring existing user-defined subclasses. Those subclasses will be defined with the release TimeFormat API that calls for a property named value for getting the value. Therefore just changing all of the built-in classes to use to_value will not help the user subclasses.

I've pushed the new strategy which is defining a base method to_value(self, parent=None) that just returns self.value by default. That's how we retain API compatibility. But classes that have extended interface possibilities will override to_value and then set value = property(to_value).

In this way the diff footprint went down a lot, at the expense of a little added conceptual complexity.

mhvk · 2015-11-17T16:00:23Z

@taldcroft - it is indeed a little more indirect, but it does seem the best solution for ensuring nobody's code breaks. Do you think it is necessary to update the general documentation? My own sense is that with your to_value docstring, it is not necessary, but I thought I should bring it up in case you forgot and felt differently.

taldcroft · 2015-11-17T16:16:20Z

@mhvk - the section on writing a custom format seems ok as is to me. As it says, to get much deeper it just makes sense to get into the code.

taldcroft · 2015-11-17T18:24:51Z

@mhvk - are you good with this now?

mhvk · 2015-11-17T18:55:42Z

Yes, it all looks OK. Am merging...

Time value attribute incorrect when scale != epoch_scale

Time value attribute incorrect when scale != epoch_scale Conflicts: astropy/time/formats.py astropy/time/tests/test_basic.py

taldcroft added time Bug Effort-low Affects-release 💤 Package-intermediate labels Nov 16, 2015

taldcroft self-assigned this Nov 16, 2015

Fix bug in TimeFromEpoch and improve code separation

d537c4d

taldcroft assigned mhvk and unassigned taldcroft Nov 16, 2015

taldcroft added this to the v1.0.7 milestone Nov 16, 2015

Update change log [skip ci]

714048e

mhvk reviewed Nov 16, 2015
View reviewed changes

Implement suggestions from @embray @mhvk

a99c33e

mhvk reviewed Nov 16, 2015
View reviewed changes

Revert some of the changes for back-compatibility

1b31a2a

mhvk removed Effort-low 💤 Package-intermediate Priority-Medium labels Nov 17, 2015

mhvk added a commit that referenced this pull request Nov 17, 2015

Merge pull request #4312 from taldcroft/time-epoch-bug

7b6a260

Time value attribute incorrect when scale != epoch_scale

mhvk merged commit 7b6a260 into astropy:master Nov 17, 2015

mhvk added a commit that referenced this pull request Nov 19, 2015

Merge pull request #4312 from taldcroft/time-epoch-bug

23b845c

Time value attribute incorrect when scale != epoch_scale

mhvk added a commit that referenced this pull request Dec 4, 2015

Merge pull request #4312 from taldcroft/time-epoch-bug

c163176

Time value attribute incorrect when scale != epoch_scale Conflicts: astropy/time/formats.py astropy/time/tests/test_basic.py

taldcroft deleted the time-epoch-bug branch October 1, 2019 20:47

ReinhardPrix mentioned this pull request Oct 24, 2026

Add a scale-dependent gps_coord format analogous to MJD #20709

Open

Conversation

taldcroft commented Nov 16, 2015

Uh oh!

mhvk Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

embray Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

embray Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

mhvk commented Nov 16, 2015

Uh oh!

embray commented Nov 16, 2015

Uh oh!

taldcroft commented Nov 16, 2015

Uh oh!

taldcroft commented Nov 16, 2015

Uh oh!

taldcroft commented Nov 16, 2015

Uh oh!

mhvk commented Nov 16, 2015

Uh oh!

mhvk Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

mhvk Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Nov 17, 2015

Uh oh!

mhvk commented Nov 17, 2015

Uh oh!

taldcroft commented Nov 17, 2015

Uh oh!

taldcroft commented Nov 17, 2015

Uh oh!

mhvk commented Nov 17, 2015

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants