search

LSSTDESC / imSim

GalSim based Rubin Observatory image simulation package
https://lsstdesc.org/imSim
BSD 3-Clause "New" or "Revised" License
36 stars 15 forks source link

Make compatible with GalSim v2.5 #418

Closed rmjarvis closed 11 months ago

rmjarvis commented 1 year ago

This fixes a few things that were incompatible with the upcoming GalSIm 2.5.

  1. The GalSim image processing moved where the full image is built, so we no longer need to do that in the imSim builder. That bit is now guarded with a if galsim.__version_info__ < (2,5) to still build the image if on 2.4.x, but not on 2.5.
  2. Now the result of obj * sed is a galsim.SimpleChromaticTransformation, so that required some changes to tests in test_stamp.py, which make specific assumptions about object types.
  3. There were some bug fixes in how the Simple image type works, which broke one of the PSF tests. I also realized that we weren't respecting the user specification of the image size in that case, so I fixed that.
  4. The bit where we fix the SEDs doesn't work with the new delayed SED properties. That procedure is different enough that I just made two different _fix_seds_* functions, and we copy one of them to the _fix_seds name depending on the GalSim version.

Separate from that, I was surprised that this function was still being called. @jchiang87 Can we please change the code in SkyCatalog where it makes the SED objects to use interpolant='linear' so this step doesn't have to be done? It would be faster to have them built correctly from the start rather than fix them here.

jchiang87 commented 12 months ago

Separate from that, I was surprised that this function was still being called. @jchiang87 Can we please change the code in SkyCatalog where it makes the SED objects to use interpolant='linear' so this step doesn't have to be done? It would be faster to have them built correctly from the start rather than fix them here.

Sure, you just had to ask back when you saw the issue.

rmjarvis commented 12 months ago

Great. I think we had a conversation about this back then, but it probably never got formalized and written down as an actual issue, so it slipped through the cracks. I should have posted an issue about it on the skycatalogs repo.

jchiang87 commented 12 months ago

@cwwalter and I have independently tried running this branch with GalSim 2.5 (i.e., in my case, with the current GalSim main). Bright stars seem to being rendered with small stamps instead of as bright stars with diffraction spikes. Here's a comparison showing a field with a bright star rendered using GalSim 2.4.11 and the imSim code in main (left) and GalSim 2.5 + this branch (right):

R22_S01_GalSim_2 4 11_vs_2 5

rmjarvis commented 12 months ago

Weird. I'm having trouble reproducing that behavior.

If I change the second item in the example_instance_catalog (i.e. the first one that falls on the image) to either 14.84 or 15.84, I get diffraction spikes. In both cases (this branch / galsim main or imsim main / galsim 2.4.11), the first one is FFT with stamp size 1382, and the second one is phot with stamp size 760.

Here are the images: This branch/2.5:

Screen Shot 2023-09-19 at 7 37 57 AM Screen Shot 2023-09-19 at 7 45 55 AM

Main branch/2.4.11:

Screen Shot 2023-09-19 at 7 49 12 AM Screen Shot 2023-09-19 at 7 51 24 AM

Any idea what would be different about this compared to what you're doing (presumably with the skycatalog)?

cwwalter commented 12 months ago

Have to run to class, but both Jim and I are using the skyCatalogs for this test. That is one difference.

rmjarvis commented 12 months ago

Sure. The problem is it's harder to adjust the flux with a skycat object. And there aren't any FFT stars in our example skycat file.

jchiang87 commented 12 months ago

We do have bright stars in our test data. In examples/imsim-user-skycat.yaml, setting

input.sky_catalog.obj_types: [star]

will use the skyCatalogs stars and reproduces the issue.

rmjarvis commented 12 months ago

Ah! Thanks. I didn't notice that we were restricting to only galaxies by default in that file.

rmjarvis commented 12 months ago

I tracked down the problem, and I've pushed a fix. (It's from an optimization in GalSim 2.5 that ends up changing one of our isinstance tests to not do what it used to do.)

I'd like to add a unit test about this to check that stamp sizes come out right for a range of object types. But if you want to, feel free to try again with your tests to check if it seems to be fixed there.

cwwalter commented 12 months ago

Great! Will do.

jchiang87 commented 12 months ago

Thanks, Mike! Looks good for me: galsim_2 5_stamp_fix

rmjarvis commented 11 months ago

I think I addressed all the comments from Jim. I also added a new test function that makes stamps for a bunch of different kinds of objects and checks that the sizes make sense. So if we ever hit some other change that breaks the stamp size calculations, we should notice.

I tried to hit basically every branch in that function, which required a bit of fiddling, since there aren't any hugely bright galaxies in the input db, where the bright galaxy adjustments make sense. Nor do we have any extremely faint objects. I could fiddle parameters to test some things that happen at faint fluxes. But the one branch I couldn't (at least easily) include is when the realized flux is 0, and it early exists without drawing. Probably ok, since that bit of code doesn't seem as fragile as the parts I am testing, but just mentioning that omission.

The test is failing, I think because I tuned the flux values to use the new throughputs in #422. Once that is merged, I think the test should start working. Other than that, I think this is ready to go if anyone else wants to take a look.

rmjarvis commented 11 months ago

I changed the default nbatch to 100 regardless of checkpointing. There is a comment about this in the main config file.

cf. https://github.com/LSSTDESC/imSim/pull/418/commits/7999f13e5d6e85f258fda63a470ecf76d9c32a03#diff-66cc22ba5c05239d6fa6bd21a24b044556b6bb87f3de01169eb3e8fa451e9394R127