-
Notifications
You must be signed in to change notification settings - Fork 9
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
Reintroduce SpatialVaryingPSF module as a refactored class into v2.0+ #116
Comments
@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)? |
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). |
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. :-) |
@jeipollack Sorry my bad, I just edited the comment |
@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. 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. |
@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 |
@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. |
I wanted to know if I could add you (@tobias-liaudat) as a reviewer for this module. |
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. |
@jeipollack sounds good :) Yeah, go ahead and add me |
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:
The text was updated successfully, but these errors were encountered: