CosmoStat / wf-psf

Data-driven wavefront-based PSF modelling framework.
MIT License
19 stars 9 forks source link

Reintroduce SpatialVaryingPSF module as a refactored class into v2.0+ #116

Open jeipollack opened 8 months ago

jeipollack commented 8 months ago

For generating spatially varying PSFs to serve as validation data, we need to reintroduce the module from the old main branch: https://github.com/CosmoStat/wf-psf/blob/old_main_backup/wf_psf/GenPolyFieldPSF.py

The module should be refactored to improve:

jeipollack commented 8 months ago

@tobias-liaudat why are you checking the limits of xv- and yv- elements in the way implemented at L208 as oppose to L145 (which I moved to a function)?

tobias-liaudat commented 8 months ago

I don't recall exactly.. but it seems that in the L145 case I need to enforce it strictly, while in the L208 to compute the Zks there can be some flexibility (although if it goes too far from the limit there could be a problem). I think I changed to that relaxed enforcement in L208 because with the strict enforcement from L145 I had a problem when simulating some field (quick and dirty solution).

jeipollack commented 8 months ago

Sorry I am confused because you wrote second twice in the first sentence. Maybe put the L### next to each so I can follow better. :-)

tobias-liaudat commented 8 months ago

@jeipollack Sorry my bad, I just edited the comment

jeipollack commented 8 months ago

@sfarrens and @tobias-liaudat I completed the refactoring of the spatial_varying_psf.py module with 84% unit test coverage.

I confirmed that the WFE_RMS map and Zernike polynomial coefficients are reproduced with respect to the old code. I included them as unit tests in spatial_varying_psf_test.py but for very small dimensions.

WFE_RMS_new_versus_old

I want to open a Pull Request although it's not fully integrated into WaveDiff, yet. It's just a refactored module with unit tests. Let me know what you think.

tobias-liaudat commented 8 months ago

@jeipollack Great! I think it might be good to add a script that shows how to use the refactored code to create a polychromatic spatially varying PSF dataset, in the same way I had the scripts to generate PSF datasets with the original version

jeipollack commented 8 months ago

@tobias-liaudat I thought I would save that for the next PR, which I will begin working on. Otherwise, it overwhelms the reviewer as this module features a lot of changes.

jeipollack commented 8 months ago

I wanted to know if I could add you (@tobias-liaudat) as a reviewer for this module.

jeipollack commented 8 months ago

Just to clarify, I believe it's best to focus on completing the current set of changes in a small PR to ensure thorough review and testing (as Sam taught me).

Breaking down tasks into smaller, more manageable PRs allows us to maintain a clear focus, manage review cycles effectively, and reduce the risk of errors. It also enables us to give/receive feedback in smaller increments, leading to a more agile and responsive development process. This approach aligns with best practices in software development.

tobias-liaudat commented 7 months ago

@jeipollack sounds good :)

Yeah, go ahead and add me