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

WIP: Continue to define Visibility class API #62

Closed
wants to merge 49 commits into from

Conversation

DanRyanIrish
Copy link
Contributor

This PR carries on from #55 in refining the API of a new Visibilities object. Do not merge until #55 is merged.

annavolp and others added 30 commits April 29, 2024 18:23
Make underlying data structure for Visibilities class an xarray
@DanRyanIrish DanRyanIrish marked this pull request as draft May 21, 2024 15:37
@DanRyanIrish DanRyanIrish marked this pull request as ready for review May 22, 2024 07:54
…lude optional metadata, rather than top level kwargs. Also add instrument attribute to VisMeta.
@DanRyanIrish
Copy link
Contributor Author

Thanks for the review @paolomassa. It makes no difference whether the units are defined as arcsec or deg. It just matters that the unit given bad user can be converted to the defined unit. It doesn't mean the value is actually converted to that unit. However, we should probably be consistent throughout the API and use deg or arcsec everywhere. Probably make most sense to use deg as it's the simplest/most common angular unit.

Copy link
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

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

Thank this looks good, no blockers, few small issues and the tests for the Meta side of things are on the low side.

xrayvision/visibility.py Outdated Show resolved Hide resolved
xrayvision/visibility.py Show resolved Hide resolved
xrayvision/visibility.py Outdated Show resolved Hide resolved
xrayvision/visibility.py Outdated Show resolved Hide resolved
xrayvision/visibility.py Outdated Show resolved Hide resolved
xrayvision/visibility.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 81.37255% with 38 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@b79f0bf). Learn more about missing BASE report.

Files Patch % Lines
xrayvision/visibility.py 81.37% 38 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #62   +/-   ##
=======================================
  Coverage        ?   89.26%           
=======================================
  Files           ?        7           
  Lines           ?      829           
  Branches        ?        0           
=======================================
  Hits            ?      740           
  Misses          ?       89           
  Partials        ?        0           

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

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.

4 participants