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

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

Open
jeipollack opened this issue Feb 13, 2024 · 10 comments
Open
Assignees
Labels
enhancement New feature or request needed

Comments

@jeipollack
Copy link
Contributor

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:

  • readability, i.e. update class names and methods
  • isolate tasks to enable unit testing
  • add doc strings for API documentation
@jeipollack jeipollack added enhancement New feature or request needed labels Feb 13, 2024
@jeipollack jeipollack self-assigned this Feb 13, 2024
@jeipollack
Copy link
Contributor Author

@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
Copy link
Member

tobias-liaudat commented Feb 28, 2024

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
Copy link
Contributor Author

jeipollack commented Feb 28, 2024

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
Copy link
Member

@jeipollack Sorry my bad, I just edited the comment

@jeipollack
Copy link
Contributor Author

jeipollack commented Mar 1, 2024

@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
Copy link
Member

@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
Copy link
Contributor Author

jeipollack commented Mar 1, 2024

@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
Copy link
Contributor Author

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

@jeipollack
Copy link
Contributor Author

jeipollack commented Mar 1, 2024

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
Copy link
Member

@jeipollack sounds good :)

Yeah, go ahead and add me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needed
Projects
None yet
Development

No branches or pull requests

2 participants