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

Fix the way we set date on WCS #7606

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Cadair
Copy link
Member

@Cadair Cadair commented Apr 26, 2024

We got an in-person bug report from someone trying to reproject EUI/FSI to a SPICE map. SPICE headers (apparently all SolO data) have date-beg, date-avg and date-end in their header but to "support [...] older software" they also specify date-obs with the same value as date-beg.

All this is to setup the fact that we only set dateobs on the WCS we build in map from our date property which (in order) pulls from these sources:

  1. The DATE-OBS FITS keyword
  2. .date_average
  3. .date_start
  4. .date_end
  5. The current time

This means that the WCS object we build for SPICE (which is a GenericMap) uses DATE-OBS which isn't the time corresponding to the observer location, and can in fact be out by hours.

This fix priorities setting wcs.dateavg which is more correct and should fix the specific SPICE issue.

@Cadair Cadair added Bug Probably a bug map Affects the map submodule backport 5.0 on-merge: backport to 5.0 backport 5.1 on-merge: backport to 5.1 🔥 🔥 Urgent issue or fix labels Apr 26, 2024
@Cadair Cadair requested a review from a team as a code owner April 26, 2024 10:45
@Cadair Cadair requested a review from ayshih April 26, 2024 10:56
@Cadair

This comment was marked as outdated.

@Cadair Cadair added the Needs Review Needs reviews before merge label Apr 26, 2024
@ayshih
Copy link
Member

ayshih commented Apr 26, 2024

For EUIMap, we override the .date property in the subclass such that it prioritizes DATE-AVG (#5462 and #6549), so the reason that we can't do that for the SPICE map is because we don't have a subclass for it?

I'm uneasy that this PR changes how GenericMap works for such a header, and it allows for a potential complete disconnect between what the .date property returns (DATE-OBS) and the three dates inserted into the WCS (DATE-AVG, DATE-BEG, and DATE-END). In a general case, DATE-OBS is not necessarily equal to DATE-BEG.

Also, this PR doesn't fix the SPICE problem entirely. The motivation for overriding .date for EUIMap is because the observer-location metadata in the header is specified for the DATE-AVG time rather than the DATE-OBS time (see #5450). Since this PR does not change the use of the .date property by the .observer_coordinate property, the observer coordinate is presumably inaccurate.

@nabobalis nabobalis added this to the 6.0.0 milestone May 8, 2024
@nabobalis nabobalis added No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) and removed backport 5.0 on-merge: backport to 5.0 backport 5.1 on-merge: backport to 5.1 labels May 11, 2024
@samaloney
Copy link
Contributor

samaloney commented Jun 11, 2024

So I have a header which contains these keywords

('date-beg', '2023-01-01 12:00:00.000'),
('date-avg', '2023-01-01 12:01:00.000'),
('date-end', '2023-01-01 12:02:00.000'),

created using the make_fitswcs_header which I then pass into a map with data but the wcs on the map only has date-obs

smap.wcs.to_header()
...
DATE-OBS= '2023-01-01T12:01:00.000' / ISO-8601 time of observation 

I would expect that at least the three date-beg, date-avg, and, date-end would be set on the maps wcs. If date-obs was added for compatibility that wouldn't cause a problem at least in my case. At leat on the face of it I can't see huge problem with
changing this

sunpy/sunpy/map/mapbase.py

Lines 613 to 615 in 767cd79

w2.wcs.cunit = self.spatial_units
w2.wcs.dateobs = self.date.isot
w2.wcs.aux.rsun_ref = self.rsun_meters.to_value(u.m)

to this

w2.wcs.dateobs = self.date.isot
w2.wcs.datebeg = self.date_start.isot
w2.wcs.dateavg = self.date_average.isot
w2.wcs.dateend = self.date_end.isot

Incidentally do you need a .utc I'm not sure what the fits standard expect or needs regards time scale.

For STIX we integrate images over 10 of seconds to minutes for the coordinate transforms we use the mean pointing and spacecraft position in this time range so I need to preserve at least date-beg and date-end.

Comment on lines +618 to +621
if self.date_average is not None:
w2.wcs.dateavg = self.date_average.isot
else:
w2.wcs.dateobs = self.date.isot
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that we conditionally set dateobs? Can we not always set this and then let the map source/.date decide what actually gets propagated up here?

Suggested change
if self.date_average is not None:
w2.wcs.dateavg = self.date_average.isot
else:
w2.wcs.dateobs = self.date.isot
w2.wcs.dateobs = self.date.isot
if self.date_average is not None:
w2.wcs.dateavg = self.date_average.isot

Copy link
Member

@ayshih ayshih Jun 12, 2024

Choose a reason for hiding this comment

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

The reason that @Cadair is doing this – and one reason for my discomfort with this PR – is because the SPICE map doesn't actually have a source subclass. So, he wants GenericMap to put the "correct" time(s) in the WCS despite the .date property returning the "incorrect" time.

Copy link
Contributor

Choose a reason for hiding this comment

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

So would you rather we add a spice source map?

@ayshih
Copy link
Member

ayshih commented Jun 13, 2024

I wrote up a (counter-)proposal in #7673

@samaloney
Copy link
Contributor

Could we split this up two part one that propagates the date-beg, date-avg and date-end to the map.wcs and then what ever is decided on #7673. Where the first part could at least be back ported, maybe?

@ayshih
Copy link
Member

ayshih commented Jun 17, 2024

We should be aware that backporting the first part would not be purely informational: because our WCS->frame mapping prioritizes .wcs.dateavg over .wcs.dateobs, there can be a difference in the coordinate frame for a map, and there can be a mismatch with the observer coordinate (which uses .date, and hence prioritizes DATE-OBS).

@nabobalis
Copy link
Contributor

I would rather not backport anything unless we really really have to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Probably a bug 🔥 🔥 Urgent issue or fix map Affects the map submodule Needs Review Needs reviews before merge No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants