Add new date properties to Map #5449
Conversation
dstansby
added
map
dstansby
force-pushed
the
date-props
branch
2 times, most recently
from
9559180
to
03d651a
Compare
There was a problem hiding this comment.
Just a few minor suggestions, but this all looks good to me.
A lingering worry that came up in the dev call last week: this is potentially a breaking change. If a map has both DATE-OBS and DATE-AVG (or equivalently a DATE-BEG and a DATE-END), a release following this PR will return the latter for .datewithout any sort of warning as to the change in behavior whereas 3.0 returns the former. I'm not completely sure how we would handle a deprecation here. Maybe switching the order of date_avg and date-obs in .date and then adding a deprecation warning that we will prefer date-avg in 4.1?
I would argue it's a bugfix? But maybe not, perhaps we can discuss this point again on this weeks' call. |
sunpy/map/mapbase.py
Outdated
| return parse_time(time, scale=timesys.lower()) | ||
|
|
||
| @property | ||
| def date_beg(self): |
There was a problem hiding this comment.
I see no reason to tie ourselves to the weird short FITS names?
| def date_beg(self): | |
| def date_start(self): |
sunpy/map/mapbase.py
Outdated
| return self._get_date('date-end') | ||
|
|
||
| @property | ||
| def date_avg(self): |
There was a problem hiding this comment.
| def date_avg(self): | |
| def date_average(self): |
or maybe
| def date_avg(self): | |
| def date_center(self): |
There was a problem hiding this comment.
I think worth sticking with average, given this is the wording in FITS and this doesn't always have to be the center of the observation interval?
| # Get the time scale | ||
| if time is not None and 'TAI' in time: | ||
| if 'TAI' in time: |
There was a problem hiding this comment.
I think this bit of logic should remain solely in .date.
There was a problem hiding this comment.
Thinking about this, it should probably live in map sources? But given it's just raising a warning, I think worth keeping it for all the date fields?
|
Some more decisions:
|
See #5394 (comment), which summarises a recent chat on the dev call.
Still needs: