Various cleanup in Optical class - #156
Conversation
4fe7424 to
3de16db
Compare
|
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? |
|
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. |
aaronroodman
left a comment
There was a problem hiding this comment.
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.
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.
galsim.Aperturefigure 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.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 singleself.apertureattribute, so it won't load multiple times regardless.mirror_figure_screenwas 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.mirror_figure_halfsizeparameter is unnecessary if the image has the right pixel scale stored (like we do withpupil_plane_im). So I set the pixel scale appropriately in the file and changed the implementation to use that rather thanmirror_figure_halfsize.mirror_figure_scaleanalogous topupil_plane_scaleto allow the user to override the value in the image file.pupil_plane_imandmirror_figure_imvalues inoptical_templateswere 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 adata_dirvalue for Piff, accessible aspiff.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.optical_templatesa 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.des_simpleanddes_paramwere identical. So I removed the latter. (We can switch the name todes_paramif people prefer, but let's just have one dict with those values.)gsparams_templatesitems 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.starby2andstardon't really tell me what they mean. I changed them tofastandfasterrespectively, with some commentary about what approximations are involved to make the run time faster when using them.