New sensor models and auto-switch for ITL or e2v - #493
Conversation
|
with I haven't yet been able to reproduce this on either my own Mac or Perlmutter where all the tests complete successfully, but I do find that the FWHMs reported were passing and consistent across different LSST pipelines releases until at least w_2025_07, then changing:
The values above are identical across both main and this PR's branch for a given pipelines release. That last pair of FWHMs looks very close to the ones which caused the test to fail in the latest CI run. I suspect that something in this particular test is sensitive to a change in the pipeline, now causing it to fail this assertion. |
|
That's very weird. Can you extract the actual images (both Perlmutter w_2025_07 and w_2025_10 perhaps?) I wonder if that'd show something obvious to check. |
|
On my machine with galsim 2.6.1, I get So it's not something with the latest galsim version. Maybe numpy changed something? Seems weird, but I can't think what else in the environment would be a likely culprit. We don't go too crazy with our dependencies in GalSIm, and the AtmosphericPSF class being used in that test doesn't seem to use any lsst stack details. So I don't think there should be too much in the dependency list that is even relevant to this. If you can reproduce this locally, it would probably be helpful to try to figure out what env changes make the problem appear. (Sorry, I know that might be quite tedious.) Maybe that will give us a clue. |
|
Thanks @jmeyers314 @rmjarvis, I'll take a look and see if anything turns up. |
|
I think I've found the problem. The images are identical (thanks for the suggestion!) so I looked into how the FWHM itself was being calculated from them. It turns out that how |
|
Looks like there's a kind="stable" argument to argsort. Bet that'd help. |
|
Ah. I see you already mentioned that in the other thread... |
|
I'm looking into how best to address the GalSim issue, but for this imsim test, I think you can just remove the FWHM test. The MomRadius test is already doing a sufficient check, so I don't think we also need the FWHM one. |
…nt sensor models.
…less overridden in config.
…rent sensor models.
a024b53 to
c8c50c8
Compare
|
I've just rebased onto main after merging #494. |
rmjarvis
left a comment
There was a problem hiding this comment.
LGTM. Just one very minor suggestion.
| assert image2.array.max() <= image0.array.max() | ||
| assert image3.array.max() <= image0.array.max() | ||
| assert image4.array.max() <= image0.array.max() | ||
| assert image2.array.max() <= image1.array.max() |
There was a problem hiding this comment.
I think you could probably just test that image0 and image1 are equal above and then only test 2,3,4 against one of them, rather than duplicating these tests for 0 and 1.
There was a problem hiding this comment.
Yes, that's probably overkill! I've just condensed these as requested.
|
Thanks! Merging in now. |
Craig Lage has provided us with new models of the ITL and e2v sensors used in LSSTCam which use four points along each pixel edge; previous models (provided with GalSim) use 8 and 32 points. Using lower resolution sensor models saves memory, which is particularly helpful for the photon pooling pipeline in which the full detector has to be retained in memory.
I've added all the models, including those we had previously, to the imSim repository. A new pair of tests (heavily inspired by GalSim's test_sensor.py) check the various models and ensure that the spot size doesn't differ more than would be expected across the different model resolutions, and regression tests are there on the second moments and ellipticities of generated in the test images.
Finally, I noticed while doing this that imSim was always using the default
SiliconSensormodel from GalSim, which is the 8-point ITL model. I added a little logic tolsst_image.pyin the image setup that switches the sensor between the 4-point ITL and e2v models according to which LSSTCam detector is being simulated -- unleass a sensor model name has already been given in the config, in which case that's used instead.