search

lsst-ts / ts_phosim

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

DM-30128: Update ts_phosim to work with latest version of ts_ofc #50

Closed tribeiro closed 3 years ago

tribeiro commented 3 years ago

Some notes for the reviewer:

tests/test_skySim.py::testAddStarByChipPos was failing due to some issue with upstream packages. Either some change in the DM stack or ts_wep is causing the failure. I update the values so the test will pass, if we can fix this in the upstream I can remove the patch before merging.

I added conda recipe files but the build will not work unless we run it with --no-test. We still need to figure out how to make builds for packages that depend on the stack, but I thought it would be nice to have the recipe added here already. If you disagree I can remove them as well.

teweitsai commented 3 years ago

Please help to fix the bin.src/ as I reported in the JIRA ticket. After that, let us run the simulation with PhoSim to make sure everything works. Then, we could do the review after that. Thanks!

teweitsai commented 3 years ago

Based on the change of code after the previous review, I do think we need to re-review this PR after it is done. In the previous review, the WEP related code is not touched, which is not the case here. In addition, this PR contains the update of ts_wep and ts_ofc in the same time now, which does not fulfill the original spec of JIRA ticket. Actually, two separate PRs are preferred in this case.

tribeiro commented 3 years ago

@teweitsai this is ready for review again.

teweitsai commented 3 years ago

@tribeiro Do you have any image (simulation result) can show me that the OPD and image closed simulation can work for ComCam and LSST FAM? For the image part, did you test the amplifier images and eimages? In addition, for the image simulation, did you test the conditions of boresight that is not (0, 0) and filter is not the reference filter? Thanks!

BTW, it looks like there is failure of unit test.