search

lsst-ts / ts_phosim

High-Level Module to Perturb the PhoSim
GNU General Public License v3.0
0 stars 1 forks source link

Zernike -> FWHM metric [DM-33115] #65

Closed jfcrenshaw closed 1 year ago

jfcrenshaw commented 2 years ago

Added function to convert Zernike amplitudes (in microns) to their quadrature contributions to the PSF FWHM (in arcseconds). This is to address ticket DM-33115.

I wasn't exactly sure where to put this, so I put it in its own module/function in utils. I figured it would be good to open a pull request with the implementation in place, and someone more familiar with ts_phosim could tell me if and where to move it.

teweitsai commented 2 years ago

Please help:

  1. Update the version history file for this new feature.
  2. Put the license details in the new file if you want to put your new function in that file.
  3. Put the parameters into the policy directory if possible.
  4. Add the unit test to the new functions.
  5. Make sure the pull request can pass the Jenkins tests.
  6. Make sure your doc string follows the format of numpy.
  7. I think there are some existed enum already when you check the available instrument. Try to use the existed enum if possible: https://github.com/lsst-ts/ts_wep/blob/develop/python/lsst/ts/wep/Utility.py#L42-L47

Thank you!

jfcrenshaw commented 2 years ago

@teweitsai I believe I have satisfied all of the points above, with one exception:

  1. Put the parameters into the policy directory if possible.

I looked in the policy directory, but I'm not really sure what to do here. Are these all yaml files specifying defaults? Could you give me a little advice on what to do here?

Thanks for your review!

teweitsai commented 2 years ago

You can create a new yaml file or put the parameters into existed yaml file (if there is one that you think it is a good place). You can add comments to explain the new parameters. Thanks!

jfcrenshaw commented 2 years ago

Is this what you were thinking? There is only one parameter that can be defaulted.

https://github.com/jfcrenshaw/ts_phosim/blob/73f4f066e353cbc70dea2625636ae3b76a071dda/policy/convertZernikesToPsfWidthSetting.yaml#L1-L2

Or were you saying I should put the conversion factors in the yaml? https://github.com/jfcrenshaw/ts_phosim/blob/73f4f066e353cbc70dea2625636ae3b76a071dda/python/lsst/ts/phosim/utils/ConvertZernikesToPsfWidth.py#L93-L170

teweitsai commented 2 years ago

I will put all the conversion factors in the yaml instead of hard-coding them. The camType is not the parameter but the argument of function (or the optional argument if you like). In addition, for the enum value, it is better to use the name instead of value in the yaml file. Otherwise, it is hard to understand what it is and what might be the available options.

jfcrenshaw commented 2 years ago

Okay the policy file now contains the conversion factors.

Assuming the CI passes again, is this ready to merge?

teweitsai commented 2 years ago

No. You need to put the "Reviewers" to ask for the review of pull request. Then, after the approve, you can merge the pull request. What I did is called the "comment" instead of "review".

In addition, it is better to rewrite the git history (to organize the comments) before asking the review or pull request.

jfcrenshaw commented 2 years ago

I cannot request reviews in the "Reviewers" field above because I do not have write access to this repo. But I have sent a slack message to @jbkalmbach asking him to review.

jbkalmbach commented 2 years ago

One more comment now that I think about it some more. Do we want to change the file name to a more general name for Zernike analysis in case we want to add some more code in there to calculate metrics like we talked about at the last AOS meeting?

jfcrenshaw commented 2 years ago

One more comment now that I think about it some more. Do we want to change the file name to a more general name for Zernike analysis in case we want to add some more code in there to calculate metrics like we talked about at the last AOS meeting?

  1. Any suggestions for a name change?
  2. I think some metrics are already included in OpdMetrology.py, e.g. calcPSSN, calcFWHMeff, calcDm5. Maybe instead, this conversion should be moved there too? I'm just not sure how the OpdMetrology class gets used, since the docstring is pretty bare.
jbkalmbach commented 2 years ago

Yeah, it does look like if we make some sort of metrics utility we'll want those there too. So maybe let's just leave things the way you have them now and think about this some more after we start to converge on what the most useful metrics are as we go forward.

jfcrenshaw commented 2 years ago

@jbkalmbach sounds good. I will squash the history again, and then merge this addition.

I mentioned unifying the metrics as a future goal to Andy. He said he expects ts_phosim will be retired as we move away from phosim, and that the important pieces will need to be moved over to other packages. So maybe that is the stage where we should unify the metrics into some common framework.

jfcrenshaw commented 1 year ago

Superseded by PR #72