-
Notifications
You must be signed in to change notification settings - Fork 6
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
make SkyModel inherit from UVBase, adjust accordingly #49
Conversation
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
+ Coverage 91.40% 95.19% +3.78%
==========================================
Files 4 4
Lines 512 853 +341
==========================================
+ Hits 468 812 +344
+ Misses 44 41 -3
Continue to review full report at Codecov.
|
@aelanman Ok, I think this PR is finally ready for you to look at. |
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.
This looks great! Thank you for all of this work. My comments are mostly questions and a few nitpicks. I think the biggest thing we'll have to figure out following this is how to handle the votable catalog reading in pyuvsim again.
I haven't looked very closely at the changes to test_skymodel.py, but from what I can tell they look fine. I'd like to try running the reference simulations with these changes to see if they break anything before approving. We may need to include a small patch to pyuvsim just to get the gleam catalog to work, before doing a larger overhaul of how catalogs are specified in configuration files.
I took a general scroll through and things seem to make sense to me, nothing glaring stuck out. I can give it a much finer toothed comb after lunch today. |
I was able to look more closely. Changes Look nice. |
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.
Thanks for addressing my comments. I just have a couple more minor things I noticed, but this should be ready to go.
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.
This looks good! Thanks for doing all this.
@bhazelton I've changed |
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.
This all looks good to me now, but Bryna should take a look at the test I changed.
@aelanman your test looks good. I added a round-tripping test for the rise/set_lst parameters through the conversion to a SkyModel object and back. I also added support (and a test) for MoonLocation type telescope locations. |
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.
Looks good! Thanks.
Description
Make SkyModel inherit from UVBase. This highlighted some inconsistencies with how we were handling different spectral types. This PR clarifies that and makes a number of changes to fully support all the different spectral types.
Motivation and Context
closes #37
closes #24
closes #20
closes #12
partially addresses #8 -- Flux still needs units.
Checklist: