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

Issue 256 #264

Merged
merged 11 commits into from
Jun 7, 2024
Merged

Issue 256 #264

merged 11 commits into from
Jun 7, 2024

Conversation

astrofle
Copy link
Collaborator

fix: add support for AzEl observations

Adds check for empty RADESYS column and functionality to handle AltAz (AzEl) coordinate frame

Still needs tests, but it can handle Issue #256.

Adds check for empty radesys and functionality to handle AltAz (AzEl) coordinate frame
Copy link
Collaborator

@etsmit etsmit left a comment

Choose a reason for hiding this comment

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

Two things:

  1. Add more to the warning (either at the GBTFITSLoad step or when changing velocity frames) that the Az/El values are invalid and will result in an incorrect frequency shift. This could be detected as a separate case from drift scans by detecting when e.g. elevation <4.9. (Or when az/el values are unchanging?)
  2. Possibly add the scan numbers corresponding to the blank radesys rows to the warning message, since the antenna manager can be taken in/out over the course of a session. For example, "No RADESYS specified for scans X,Y,Z; assuming AltAz"

@astrofle
Copy link
Collaborator Author

astrofle commented Jun 6, 2024

@etsmit I think I implemented your comments. Let me know if you think something else is needed.

Note for the future. This "fix" updates the 'RADESYS' column because that is what was being used downstream by the Spectrum objects to define their celestial coordinates. However, the definition of the 'RADESYS' column (Equatorial coordinate system name) makes it clear that this is not the right column to use to define the coordinate system (it only makes sense for equatorial coordinates). It might be better to use 'CTYPE2' and 'CTYPE3' to define the celestial coordinate system (this would avoid changing the definition of SDFITS), and only do this when a Spectrum object is being instantiated (to avoid the additional computations this fix introduces).

I will also note that:
/home/dysh/example_data/hadec/data/AGBT18B_342_01.raw.vegas/AGBT18B_342_01.raw.vegas.A.fits has a jump in elevation during scan 15, going from ~57 deg to 0 deg. No idea how @etsmit did that, but it is worth noting that in these cases we have no checks when doing operations like time averaging.

@astrofle astrofle requested review from etsmit and mpound June 6, 2024 12:45
@astrofle
Copy link
Collaborator Author

astrofle commented Jun 6, 2024

The last update also adds support for HaDEC observations.

src/dysh/fits/gbtfitsload.py Outdated Show resolved Hide resolved
@etsmit etsmit self-requested a review June 6, 2024 13:51
@astrofle
Copy link
Collaborator Author

astrofle commented Jun 7, 2024

@mpound is this fix acceptable?

Copy link
Collaborator

@mpound mpound 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.

@astrofle astrofle merged commit a03c9a6 into release-0.3.0 Jun 7, 2024
14 checks passed
@astrofle astrofle deleted the issue_256 branch July 8, 2024 18:30
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