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

define visibility class API #55

Merged
merged 51 commits into from
May 24, 2024
Merged

define visibility class API #55

merged 51 commits into from
May 24, 2024

Conversation

annavolp
Copy link
Contributor

This pull request defines a new object for visibilities and its API.

Copy link

codecov bot commented Apr 29, 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      #55   +/-   ##
=======================================
  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.

@DanRyanIrish
Copy link
Contributor

DanRyanIrish commented Apr 30, 2024

Having reflected overnight, I think there is a more structured way to organise our Visibilities API that will give us more flexibility in the future.

  • Create a VisibilitiesBase class (like Shane's BasicVisibility suggestion) that includes the bare minimum for a workable visibilities object.
    • A VisibilitiesBaseABC class defines the minimum API:
      • visibilities
      • u
      • v
      • names
      • uncertainty (optional, no format restrictions)
      • meta (optional, no format restrictions)
    • Slicing by label should also be implemented on the base class.
    • Also, implement following on VisibilitiesBase, outside of the ABC, but is needed by the VisibilitiesABC below:
      • amplitude
      • phase
      • amplitude_uncertainty
      • phase_uncertainty
  • Create a Visibilities class that inherits from VisibilitiesBase and adds extra capabilities.
    • Its API is defined by VisibilitiesABC, which inherits from VisibilitiesBaseABC. The additional following API includes:
      • amplitude
      • phase
      • meta (must be instance of VisMetaABC)
  • The minimum API of Visibilities.meta is defined by VisMetaABC which includes the following:
    • center
    • observer_coord
    • time_range
    • energy_range

The advantages of this structure include:

  • A minimum viable version of VisibilitiesBase allows images to be made as arrays, i.e. without real world coords.
  • Map images can be created if required metadata is known using Visibilities
  • Visibilities can be subclasses and the amplitude, phase, and associated uncertainty properties can be overridden, e.g. for RHESSI where uncertainties on the visibility components may not be known, while the amplitude and phase uncertainties may be.
  • Subclasses of VisMetaABC allow lots of additional metadata to be added to Visibilities, e.g. live time, background information, etc., while maintaining a dependable API for the minimum required API.

@DanRyanIrish
Copy link
Contributor

@annavolp Can you push your latest work including the tests you wrote in Dublin?

@samaloney
Copy link
Member

So I've realised two "coordinates" are needed, see the other open PR #58, both a phase_centre and offset or centre - 99% of the time phase_center will be (0,0) but you can have other values if you look at the test_phase_centre_equivalence() give an example.

@samaloney samaloney marked this pull request as ready for review May 21, 2024 15:43
@DanRyanIrish
Copy link
Contributor

If we agree that #62 is ready to be merged now, it probably makes sense to combined that PR into this one first and then merge both Anna and my contributions together. I think we're basically there. Of course, small changes can still be made in the future if we want.

@DanRyanIrish
Copy link
Contributor

@samaloney This is ready to merge.

xrayvision/visibility.py Outdated Show resolved Hide resolved
xrayvision/visibility.py Outdated Show resolved Hide resolved
xrayvision/visibility.py Outdated Show resolved Hide resolved
@DanRyanIrish
Copy link
Contributor

@samaloney Actually last minute change. We should make "energy" "spectral".

@samaloney samaloney merged commit 8580373 into TCDSolar:main May 24, 2024
13 checks passed
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