-
Notifications
You must be signed in to change notification settings - Fork 29
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 MWA Efield beam sign #1510
Fix MWA Efield beam sign #1510
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1510 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 63 63
Lines 21849 21849
=======================================
Hits 21834 21834
Misses 15 15 ☔ View full report in Codecov by Sentry. |
7ccc573
to
e7491e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Uh-oh, looks like I took too long and there are now conflicts in the changelog... If you can fix, I'll approve (quickly). |
Also better labelling for beam plots with East and North
e7491e0
to
d06c6fc
Compare
I assume failing on hera_cal shouldn't block a merge? |
Description
We recently came up with a better way to check E-field beam signs. This led me to realize we had a sign flip in the azimuthally aligned response of the MWA beam. I tracked this down as a clear mistake in translating from the mwa_pb repo so I fixed it and added our new test (which fails on main).
I also realized that there was an error in the beam plots in our tutorials sourced by an unexpected (at least to me) default in
imshow
: the (0,0) element of the image is placed in the upper left rather than the lower left (which was my expectation). This is fixable via theorigin
keyword, so I set that for all theimshow
plots in the tutorials and also improved the labelling of the beam plots to clearly indicate where East and North are.I also snuck in a couple beam-related docstring fixes I noticed recently.
Motivation and Context
Types of changes
Checklist:
Bug fix checklist:
Documentation change checklist: