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

Various cleanup in Optical class #156

Merged
merged 13 commits into from
Jul 6, 2023
Merged

Various cleanup in Optical class #156

merged 13 commits into from
Jul 6, 2023

Conversation

rmjarvis
Copy link
Owner

While working with aspects of the optatmo3 branch (cf. PR #152), I noticed a few things that I think were errors or problems (some quite minor) with the implementation. This PR fixes them.

  1. The pixel scales stored in the pupil plane images weren't right. But the implementation didn't use it -- it read in the image array and let galsim.Aperture figure out the scale automatically. We had a test that was intended to check the pixel scale and fix it if it was wrong, but it wasn't correct. So I fixed that test, which updated the pixel scales in the files to the right values.
  2. Once the pixel scales are right, then Piff can just pass the file name to galsim.Aperture, rather than doing a bunch of that work ourselves. There was a comment that the extra code was meant to avoid multiple file loads -- but it already gets cached as a single self.aperture attribute, so it won't load multiple times regardless.
  3. The construction of the mirror_figure_screen was hardcoded to 512x512, which would be an error if someone passed it an image that wasn't this size. (We currently only have one example, which is this size.) Now it builds the u and v arrays to match the size of the image.
  4. The mirror_figure_halfsize parameter is unnecessary if the image has the right pixel scale stored (like we do with pupil_plane_im). So I set the pixel scale appropriately in the file and changed the implementation to use that rather than mirror_figure_halfsize.
  5. I added an optional mirror_figure_scale analogous to pupil_plane_scale to allow the user to override the value in the image file.
  6. The pupil_plane_im and mirror_figure_im values in optical_templates were hardcoded to the relative path one needs when running in the test directory. This works for the unit tests, but would fail if a user tries to run it anywhere else. Furthermore, the image files are not installed with Piff, so it's potentially complicated for a user to know what directory to specify for these. To fix this, I implemented a data_dir value for Piff, accessible as piff.meta_data.data_dir, which points to a share directory, which we now install with piff source code. This is the same pattern we use in GalSim and imSim for installing data files. Now Piff will find the files in that directory without the user needing to specify the full path.
  7. I cleaned up the optical_templates a bit, and added a one-line overview of each one to help guide a user who might be trying to figure out which one is appropriate.
  8. In doing that, I noticed that des_simple and des_param were identical. So I removed the latter. (We can switch the name to des_param if people prefer, but let's just have one dict with those values.)
  9. Two of the gsparams_templates items are not only equal to each other, but are also equivalent to just using None (i.e. use the default GalSim GSParams). So I removed them.
  10. I also find the names of the other two confusing. starby2 and star don't really tell me what they mean. I changed them to fast and faster respectively, with some commentary about what approximations are involved to make the run time faster when using them.
  11. There were some other minor whitespace edits, mostly wrapping long lines to 100 characters to make them easier to read.

@rmjarvis rmjarvis requested a review from aaronroodman June 16, 2023 18:56
@rmjarvis rmjarvis force-pushed the pupil_plane_scale branch from 4fe7424 to 3de16db Compare June 16, 2023 22:23
@aaronroodman
Copy link
Collaborator

I checked that I get the same results making donuts, which tests the pupil function and mirror figure. So this looks good. Is a button somewhere to do the review?

@rmjarvis
Copy link
Owner Author

Click on "Files Changed" near the top of this page.

Then anywhere you want to make a comment on the code, click on the left side of the line (a blue + will appear) and type the comment. For the first comment, click "Start a review". After that, it will be "Add review comment".

Once you are done making comments, at the upper right, click "Finish your review" to post all the comments. Until then, you can go back and edit or delete comments you have queued up.

If you don't want to make any line comments, you can also just click the upper right ("Review changes") and give a single overall comment.

Copy link
Collaborator

@aaronroodman aaronroodman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I successfully revised my test notebook to work with these changes, also verifying that big donuts show the same pupil and mirror figure to confirm that the pixel scale changes to the fits files delivers identical results.

@rmjarvis rmjarvis merged commit 50b8bdd into main Jul 6, 2023
@rmjarvis rmjarvis deleted the pupil_plane_scale branch July 6, 2023 16:27
@rmjarvis rmjarvis added this to the Version 1.4 milestone Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants