Repository navigation
Add a to_value method to TimeDelta. - #8762
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8762 +/- ##
==========================================
+ Coverage 86.96% 86.96% +<.01%
==========================================
Files 400 400
Lines 59448 59450 +2
Branches 1100 1100
==========================================
+ Hits 51697 51700 +3
+ Misses 7110 7109 -1
Partials 641 641
Continue to review full report at Codecov.
|
| return u.Quantity(self._time.jd1 + self._time.jd2, | ||
| u.day).to(*args, **kwargs) | ||
|
|
||
| def to_value(self, *args, **kwargs): |
There was a problem hiding this comment.
In general I prefer to have the actual function signature here instead of just delegating to whatever Quantity.to_value() takes. While the current approach makes this method more future-proof for Quantity.to_value API changes (probably not likely), it makes it more opaque for the user and typing dt.to_value? will give no useful help.
At the very least, need a docstring that provides information that this is calling Quantity.to_value, but preferably also shows the unit and equivalencies parameters. BTW, are there any equivalencies relevant for time delta units?
There was a problem hiding this comment.
I just copied from .to above, but I agree with your sentiment and will update it. (And, no, don't think there are astropy-defined equivalencies that are relevant, but of course a user might put in anything...)
There was a problem hiding this comment.
Of the top of my head, I can think of time / frequency equivalency. Transforming a timedelta into a frequency.
There was a problem hiding this comment.
@taldcroft - I now updated the TimeDelta.to_value and .to methods with proper docstrings.
Makes using
TimeDeltaeasier in places where aQuantitywith units of time could be used. Does raise a more general question of how far one should extend this, and whether one should do the same forColumn. But seems good in itself and would have been done at the time ifQuantityhad had theto_valuemethod already.fixes #8732