ArcetriAdaptiveOptics / arte

Arcetri Random sTuff collEction
MIT License
2 stars 0 forks source link

Modal expansion: added KL, RBF and Modal Coefficients, hopefully without break everything #37

Closed imapeartree closed 2 months ago

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 90.56317% with 62 lines in your changes missing coverage. Please review.

Project coverage is 75.72%. Comparing base (8366bf6) to head (9e76262).

Files Patch % Lines
arte/utils/karhunen_loeve_generator.py 75.23% 26 Missing :warning:
test/utils/modal_decomposer_test.py 85.41% 14 Missing :warning:
arte/utils/rbf_generator.py 85.71% 10 Missing :warning:
arte/types/modal_coefficients.py 91.89% 3 Missing :warning:
arte/utils/modal_decomposer.py 84.21% 3 Missing :warning:
arte/utils/base_modal_decomposer.py 98.00% 2 Missing :warning:
arte/utils/zernike_decomposer.py 92.85% 1 Missing :warning:
test/types/modal_coefficients_test.py 97.61% 1 Missing :warning:
test/utils/karhunen_loeve_generator_test.py 97.43% 1 Missing :warning:
test/utils/rbf_generator_test.py 98.41% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #37 +/- ## ========================================== + Coverage 75.09% 75.72% +0.62% ========================================== Files 172 182 +10 Lines 11160 11729 +569 ========================================== + Hits 8381 8882 +501 - Misses 2779 2847 +68 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alfiopuglisi commented 2 months ago

If I understand correctly, the idea is to generalize the ModalDecomposer class to allow different modal bases (Zernikes, KL, etc). The idea is sound, but it looks to me like the implementation is done backwards: there should be a generic ModalDecomposer class with the interface, and then either:

I prefer the composition approach, and the generator classes are already there it seems.

alfiopuglisi commented 2 months ago

Reorganized ModalDecomposer to have a base class BaseModalDecomposer with all the rec. generating and projection code, independent from the modal base, and a series of generators for Zernike, KL and RBF modes. Small classes ZernikeModalDecomposer, KLModalDecomposer and RFBModalDecomposer are very small classes derived from BaseModalDecomposer and only adding the modal generator instantiation.

The old ModalDecomposer has been kept without any interface changes, for backward compatibility.

alfiopuglisi commented 2 months ago

@lbusoni please review (your requested changes are blocking the pull request)