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

Bugfix transforms add support for arrays of obtstime #128

Merged
merged 9 commits into from
Jun 15, 2024

Conversation

samaloney
Copy link
Member

@samaloney samaloney commented Jun 11, 2024

The sunpy/astropy coordinate stack expects SkyCoords to be able to take arrays of values but also arrays of times. This PR modifies the code to work with an array of times or values for example the flare flag positions. To support both a mean pointing between two times and and instantaneous (always interpolated between two nearest aux-epheris data point) this PR adds a new property obstime_end to the STIXImaging frame if set the average pointing, position between obststime and obstime_end is used for coordinate transforms if obstime_end = None then an interpolated pointing and position is used.

This all works nicely enough but it can introduce some inconsistencies you use a mean to transform one way but an interpolated value else where as shown in the example below.

import numpy as np
import astropy.units as u

from astropy.coordinates import SkyCoord
from astropy.time import Time
from sunpy.coordinates.frames import Helioprojective

from stixpy.coordinates.frames import STIXImaging

times = Time("2023-01-01") + [0, 2] * u.min
time_avg = np.mean(times)
stix_coord = SkyCoord(0 * u.deg, 0 * u.deg, frame=STIXImaging(obstime=times[0], obstime_end=times[-1]))

hp = stix_coord.transform_to(Helioprojective(obstime=time_avg))
stix_coord_rt = hp.transform_to(STIXImaging(obstime=times[0], obstime_end=times[-1]))

stix_coord_rt_interp = hp.transform_to(STIXImaging(obstime=time_avg))

stix_coord_rt.Tx, stix_coord_rt.Ty
# (<Longitude 0. arcsec>, <Latitude -1.39770469e-15 arcsec>)

stix_coord_rt_interp.Tx, stix_coord_rt_interp.Ty
# (<Longitude -0.05497178 arcsec>, <Latitude 0.06092345 arcsec>)

The concept would be for imaging would always use STIXImaging(obstime=<start_time>, obstime_end=<end_time>) and for other things e.g. CFL use in STIXImaging(obstime=<start_time>

times = Time("2023-01-01") + np.arange(10) *u.min
stix_coord = SkyCoord([0]* 10 * u.deg, [0]*10 * u.deg, frame=STIXImaging(obstime=times)
hp = stix_coord.transform_to(Helioprojective(obstime=time_avg))
hp.Tx, Hp.Ty
# (<Longitude [32.95393778, 32.84938953, 32.95559279, 33.24362131,
#              33.10094168, 32.9070626 , 32.99613718, 32.81095968,
#              33.16008362, 33.05861579] arcsec>,
#  <Latitude [54.87056945, 54.46366687, 54.55211655, 54.83474543, 54.50710399,
#             54.22479134, 54.36713584, 54.37145252, 54.78612434, 55.00276103] arcsec>)

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.60%. Comparing base (9315811) to head (27b3463).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
stixpy/coordinates/frames.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   72.30%   73.60%   +1.30%     
==========================================
  Files          31       32       +1     
  Lines        1892     1940      +48     
==========================================
+ Hits         1368     1428      +60     
+ Misses        524      512      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samaloney samaloney changed the title Bugfix transforms not supporting arrays of obtstime Bugfix transforms add support for arrays of obtstime Jun 12, 2024
@samaloney
Copy link
Member Author

The windows test fail is unrelated so can be ignored.

Copy link
Contributor

@paolomassa paolomassa left a comment

Choose a reason for hiding this comment

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

Looks good to me. Are the results consistent with IDL? If needed I can test it

@samaloney
Copy link
Member Author

samaloney commented Jun 12, 2024

At least in terms of the pointing information they agreement really well.

python

get_hpc_info(t[0], t[1])

IDL

aux_fits_file = stx_get_ephemeris_file(times[0], times[1], out_dir='.')
aux_data = stx_create_auxiliary_data(aux_fits_file, times, /silent)

IDL in in bold

Time Range x y roll
2023-04-19T15:52:00 to 2023-04-19T15:53:00 22.683 / 22.6462 97.908 / 97.888 6.179 / 6.179
2022-03-24T12:30:00 to 2022-03-24T12:32:00 28.977 / 28.9776 106.598 / 106.599 4.116 / 4.116
2023-03-19T22:02:00, 2023-03-19T22:04:00 -8.608 / -8.608 62.219 / 62.220 -49.851 / -49.85
'2022-08-28T16:00:00, 2022-08-28T16:04:00 25.923 / 25.923 58.179 / 58.179 -3.373 / -3.373
2022-08-28T15:20:30, 2022-08-28T15:22:30' 25.954 / 25.954 57.908 / 57.908 -3.374 / -3.374
2023-05-16T17:20:30, 2023-05-16T17:21:50' 12.460 / 12.514 84.897 / 84.844 -4.787 / -4.787
2022-03-31T18:26:20, 2022-03-31T18:27:00 -2385.997 / -2385.79 942.564 / 942.672 3.813 / 3.813

Copy link

@FredSchuller FredSchuller left a comment

Choose a reason for hiding this comment

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

At least it makes sense, and I guess you tested that it does what it is supposed to do :-)

@samaloney samaloney merged commit 310037e into TCDSolar:main Jun 15, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants