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.

Sign up for GitHub

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

Jump to bottom

Adding new Telescope attributes for feeds and mounts #1521

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

Adding new Telescope attributes for feeds and mounts #1521

wants to merge 26 commits into from

Conversation

kartographer
Copy link
Contributor

@kartographer kartographer commented Jan 25, 2025 •
edited
Loading

Adds feed orientation and mount information to Telescope class, deprecates x_orientation.

Description

A few new attributes have been added to the Telescope class: Nfeeds, which describes the number of polarizations captured by each antenna; feed_array, which records the polarizations for each antenna (one of 'x', 'y', 'l', 'r');feed_angle, which records the feed position angle (where 0 is aligned to vertical polarization); and mount_type which records the optics of the antenna. As part of putting together this PR, I've also taken a first (significant) pass at a "conventions" page for the docs, which describes some of the finer details of these new attributes (along with some other conventions pieces).

Note: This is a late-stage WIP, but I'm putting in a PR now so that interested parties can review while we conduct some additional testing. As per usual, me updating the CHANGELOG will denote when I think everything is wrapped up.

Motivation and Context

SMA is in the process of adding polarimetric support in CASA, and adding these new attributes are one of the first big steps/hurdles for that.

Closes #1470
Closes #854
Closes #1235
Closes #1425

Open items:

  • Figure out where documentation for mount_type needs to live.
  • Confirm that deprecation handling is suitable for downstream processes.
  • Extra credit: get d-terms working in CASA.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Breaking change checklist:

  • I have updated the docstrings associated with my change using the numpy docstring format.
  • I have updated the tutorial to reflect my changes (if appropriate).
  • My change includes backwards compatibility and deprecation warnings (if possible).
  • I have added tests to cover my changes.
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

@codecov Codecov
Copy link

codecov bot commented Jan 25, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.93%. Comparing base (4c01876) to head (cc592b4).

Additional details and impacted files 
@@           Coverage Diff            @@
##             main    #1521    +/-   ##
========================================
  Coverage   99.93%   99.93%            
========================================
  Files          63       63            
  Lines       21746    22044   +298     
========================================
+ Hits        21731    22029   +298     
  Misses         15       15

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

github-actions[bot]
github-actions bot reviewed Jan 28, 2025
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: d7816f1 Previous: bca6cf0 Ratio
tests/utils/test_bls.py::test_bls_to_ant[min=0-len=1] 168682.51670325958 iter/sec (stddev: 0.0002299602645136317) 1296803.9086480543 iter/sec (stddev: 0.000005510650430395054) 7.69

This comment was automatically generated by workflow using github-action-benchmark.

bhazelton
bhazelton reviewed Jan 28, 2025
View reviewed changes
Copy link
Member

@bhazelton bhazelton left a comment

Choose a reason for hiding this comment

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

This is looking interesting, I have some questions.

@kartographer
Copy link
Contributor Author

Noting here that we've had our first successful external test using CASA's polcal to derive polarimetry for a pol calibrator w/ SMA data. Also noting that CASA sends a spooky message about unknown feeds because the "antenna 0" entries are blank -- probably worth either fixing or noting somewhere in the docs...

@kartographer kartographer mentioned this pull request Feb 3, 2025
Open
@kartographer kartographer force-pushed the telescope_feeds branch 3 times, most recently from ca78d58 to a3d367b Compare February 9, 2025 17:52
@kartographer kartographer marked this pull request as ready for review February 14, 2025 17:50
@kartographer
Copy link
Contributor Author

Alright, I'm moving this out of draft since major work here is completed. Still need to do some documentation work, which I could use some guidance on where to put that, but otherwise I'm at a point where I'd like to make sure that folks are on board with what is effectively the proposed deprecation of x_orientation.

bhazelton
bhazelton reviewed Feb 14, 2025
View reviewed changes
Copy link
Member

@bhazelton bhazelton left a comment

Choose a reason for hiding this comment

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

This is looking really good to me, I'm comfortable with the approach you took for the x_orientation deprecation. Just a few other comments.

@bhazelton
Copy link
Member

I will work on PRs for the external tests that are failing, but I may not get them done until week after next when I'm back from vacation.

bhazelton
bhazelton reviewed Feb 17, 2025
View reviewed changes
Copy link
Member

@bhazelton bhazelton left a comment

Choose a reason for hiding this comment

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

LOVE the new convention page! @steven-murray can you take a look and see if anything is missing for you?

I also really like the condensed way you treated the antenna_mount in the docstring!

Just had a couple comments:

@kartographer kartographer mentioned this pull request Feb 17, 2025
Open
@steven-murray
Copy link
Contributor

@kartographer that convention doc page is fantastic, thanks! It'd be great to add a polarization convention section there as well (i.e. I=XX +YY vs I=0.5(XX + YY)).

kartographer and others added 17 commits February 18, 2025 09:22
@bhazelton
Copy link
Member

I started poking at pyuvsim to figure out what needed to be done there and realized we need to harmonize UVBeam with these changes. UVBeam doesn't have a telescope object on it, in large part because it doesn't need anything to do with antenna layout information. But it does have x_orientation, and pyuvsim uses that to set the x_orientation on the output UVData object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhazelton bhazelton bhazelton left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@telegraphic telegraphic Awaiting requested review from telegraphic

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement Telescope
Projects
None yet
Milestone
No milestone
3 participants
@kartographer @bhazelton @steven-murray