Tighten up API, tidy docs, #58
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
=======================================
Coverage ? 89.29%
=======================================
Files ? 7
Lines ? 850
Branches ? 0
=======================================
Hits ? 759
Misses ? 91
Partials ? 0 ☔ View full report in Codecov by Sentry. |
samaloney
force-pushed
the
feat-api-tidy
branch
4 times, most recently
from
d18d6f6
to
32b0213
Compare
xrayvision/tests/test_imaging.py
Outdated
| img[32, 32] = 1.0 * (apu.ph / apu.arcsec**2) | ||
| obs_vis = dft_map(img, u=u, v=v, pixel_size=[2.0, 2.0] * apu.arcsec) | ||
| img = np.zeros((65, 65)) * (apu.ph / apu.cm**2) | ||
| img[32, 32] = 1.0 * (apu.ph / apu.cm**2) |
There was a problem hiding this comment.
Aren't the units of a map photons (or counts) s^-1 cm^-2 keV^-1 arcsec^-2? I think the normalization by arcsec^2 is needed given pixel_size of dft_map is in apu.arcsec / apu.pix. The normalization by s and keV can be neglected if not considered here
There was a problem hiding this comment.
The units here don't really matter but I should double check that the result of the idft have what ever units the input visibility have with an additional arcsec^-2
There was a problem hiding this comment.
So currently the underlying code dft, idft don't really do anything with the units , specifically idft(vis, ....) will return something with the units of vis and doesn't add the 1/arcsec^2 it really should. Similarly dft(an_array, ...) doesn't check if an_array has 1/arcsec^2 in its units and just return vis with the same units as an_array.
|
Great thanks for the review @paolomassa |
samaloney
force-pushed
the
feat-api-tidy
branch
2 times, most recently
from
f702048
to
79558cf
Compare
* Optonal args are now keyword only * Use of qantity aware type annotations * Doc refaactor - page per submodule * Fix bug in transform code * Add fft equivalence test
| @@ -48,7 +50,7 @@ def time_range(self) -> Optional[Iterable[Time]]: | |||
|
|
|||
| @property | |||
| @abc.abstractmethod | |||
| def vis_labels(self) -> Optional[Iterable[str]]: | |||
| def vis_labels(self) -> Sequence[Iterable[str]]: | |||
There was a problem hiding this comment.
Since vis_labels isn't required, should this be Optional[Sequence[str]]? Or perhaps Optional[Sequence[Iterable[str]]]?
phase_centreandoffsetDemo in stixpy will have to be updated once this is merged
Closes #57, #54, #33, #37