Update unit tests for the new code installed via setup.py #259

rmjarvis · 2021-11-05T04:16:17Z

This updates some of the unit tests to work with the new code structure. Specifically, the following files now all work with imsim installed with setup.py:

test_FWHMgeom.py
test_atmPSF.py
test_batoid_wcs.py (already was working prior to this PR)
test_cosmic_rays.py
test_fopen.py
test_instcat_parser.py
test_logging.py
test_optical_zernikes.py
test_psf.py
test_tree_rings.py
test_trimmer.py

I've marked some places of concern with XXX comments. The most notable of these, I think, is about the WCS in test_instcat_parser.py. The Batoid WCS for the test instance catalog seems to be about 10 arcsec off. (Not uniformly, but that's ~ the maximum error.) That seems like more than we should have expected, so maybe @jmeyers314 could take a look at this to see if this is expected because you are doing something right now that used to be wrong, or if there is something here to be fixed.

There are more test files still to convert over, but this seems like enough of a start that it's worth people looking over the updates so far.

rmjarvis · 2021-11-12T02:19:32Z

Update: I rebased onto main to use Jim's new camera code.
Also, I managed to get github actions working with stackvana (installed with mamba), running the tests that are working so far, and the CI agrees that they are all working. :)

cwwalter · 2021-11-12T02:47:26Z

Awesome!

jchiang87

I added comments at most of your XXX questions, but didn't go through the changes in detail otherwise. I'll have a closer look, but wanted to address those questions right away.

jchiang87 · 2021-11-19T14:56:41Z

imsim/util.py

+import logging
+import sys
+
+# XXX: unneeded?


Yes, I don't think we need this any longer.

jchiang87 · 2021-11-19T14:56:58Z

tests/test_logging.py


+# XXX: I think this is probably not needed anymore.  Now use GalSim logger.



Yes, agreed.

Good. Removed this. Also test_TracebackDecorator and test_config_reader.

jchiang87 · 2021-11-19T15:43:11Z

imsim/batoid_wcs.py

+
+        Returns:
+            the constructed WCS object (a galsim.GSFitsWCS instance)
+        """

        # If a string, convert it to astropy.time.Time.
        # XXX Assumption is that string time is in scale 'TAI'.  Should make sure
        # to make this consistent with OpSim.


It looks like the rubin_sim code assumes that times are TAI by default: https://github.com/lsst/rubin_sim/blob/ad16f12bb4e5cc4ef143c6db439de6b51d10cb10/rubin_sim/utils/ObservationMetaData.py#L39

jchiang87 · 2021-11-19T15:52:46Z

imsim/instcat.py

+                        # Check for some reasons to skip this object.
+                        # XXX: Is this right?  We require dust?
+                        #      The previous code had that galactic_av = galactic_rv = 0 is invalid.
+                        #      That doesn't seem right to me...


I think the problem with Rv=0 using the lsst_sims code (and also for rubin_sim) is that there would be a divide by zero encountered (even if Av=0) at this line.

But if no dust is given in the input, can't we just not apply any extinction from dust? I.e. Av=0, Rv=1.

sure. I'm not advocating we keep things as they are. I was just giving the reason why I think the code is written this way. Same applies to my other comments.

We ran into this before:

#213

I changed this so these are not skipped, but are treated as Av=0, Rv=1.

jchiang87 · 2021-11-19T15:54:14Z

tests/test_trimmer.py

-                                             chunk_size=chunk_size)
-            self.assertEqual(len(objs[sensor]), 24)
+        # XXX: We don't use chunking anymore.  If we revisit this and put it back in,
+        #      then we can re-enable this test.


Ok with me, though I wouldn't think we'd be using the InstCatTrimmer at all anymore.

Right. I meant if we decided that the new code should be chunking like you did in the trimmer, then we might want a version of this test, modified to test the new code. I guess I'm also kind of prodding that we recall why we did chunking in the old code and think about whether it would make sense to add something similar now.

IIRC, the original reason was to avoid using up a lot of memory when reading in the instance catalogs (which covered the full focal plane) since most of the entries would be omitted once the objects that landed on the CCD were identified.

OK, I've removed this. In the new code, we already have the WCS as we're reading in the file, so there's no overhead of keeping any objects in memory that won't end up being kept.

jchiang87 · 2021-11-19T16:01:19Z

tests/test_instcat_parser.py

+
+        ra_arr = np.array([pos.ra.rad for cat in all_cats.values() for pos in cat.world_pos])
+        dec_arr = np.array([pos.dec.rad for cat in all_cats.values() for pos in cat.world_pos])
+        # XXX: These are only within 1.e-4 radians, which is kind of a lot.


Almost all of the tests in this file were written by Scott, so I think we should just update things in a way that makes sense to us.

I'm hoping @jmeyers314 can take a look at this test to see whether the previous version did make sense, and the failures are a sign of a bug or if the test isn't quite relevant anymore given the new wcs handling.

I'll take a closer look. Browsing main, it looks like these positions weren't previously tested at all? Is that right? (So maybe they were always 1e-4 rad off?)

They weren't tested on main (since we weren't running these tests), but I'm pretty sure they were tested in v1.0. That was with the older wcs code, not the BatoidWCS.

I think that is right, it used the sims code with the distortion model from for the WCS as we discussed before.

jchiang87 · 2021-11-19T16:02:52Z

tests/test_instcat_parser.py

+            for i in range(cat.nobjects):
+                obj = cat.getObj(i, bandpass=bp)
+                if 0:
+                    # XXX: The old test used the sims Sed class.  Circumventing this now,


ok, with me.

jchiang87 · 2021-11-19T16:03:28Z

tests/test_instcat_parser.py

-        self.assertGreater(valid, 10)
-        self.assertGreater(len(valid_chip_names), 5)
+        if 0:
+            # XXX: This doesn't seem relevant anymore.  But leaving this here in case we want


jchiang87 · 2021-11-19T16:03:50Z

tests/test_instcat_parser.py

+                if len(i_obj) == 0: continue
+                i_obj = i_obj[0]
+                if 0:
+                    # XXX: The old test using the sims Sed class.


jchiang87 · 2021-11-19T16:04:28Z

tests/test_instcat_parser.py

+
+        self.assertEqual(meta['gain'], 1)
+        self.assertEqual(meta['band'], 'r')
+        # XXX: These used to be nexp=2 ?? and exptime=15.  I guess per snap, but we're not doing


ok, with me.

jchiang87

Except for the one comment about removing 'Latiss' as a camera option, looks good to me.

jchiang87 · 2021-11-22T01:28:25Z

imsim/camera.py

    """
    Return an lsst camera object.

    Parameters
    ----------
    camera : str
       The class name of the LSST camera object. Valid names
-       are 'LsstCam', 'LsstComCam', 'Latiss'.
+       are 'LsstCam', 'LsstComCam', 'Latiss'. [default: 'LsstCam']


Looking at the check for telescope above,

if telescope != 'LSST': raise NotImplementedError("Batoid WCS only valid for telescope='LSST' currently")

it occurs to me that we should omit 'Latiss' from the camera options here and below.

OK. I don't actually know what Latiss is. I guess that's AuxTel maybe?
I'm fine with removing that for now, but I guess people may want to make simulations of that eventually. So we should add an issue to add that back in.

Ah, nvm I guess. That's a slitless spectrograph. Yeah, we're never going to simulate that with ImSim (at least until GalSim Issue #1018 is done). So just removing this is probably right.

Right: LSST Atmospheric Transmission Imager and Slitless Spectrograph (LATISS) on the Auxtel.

For the future I wonder if the way to do something like this would be to have dispersive elements in Batoid?

I agree we don't need to worry about LATISS now, but Batoid does have the relevant model here. The grating is removable, so it's possible to observe and simulate in imaging mode. I know roughly how to insert a dispersive element in batoid as basically a phase screen, but haven't thought through all the details yet. In particular, some kind of ray splitting must be required for different orders and some kind of throughput vs wavelength/order function must be required or emerge too.

rmjarvis · 2021-11-22T20:12:13Z

I opened Issue #262 for addressing the WCS regression tolerance being a bit large. I think everything else has been resolved, so this can probably be merged, but @cwwalter if you want more time to look it over, just say so.

cwwalter · 2021-11-23T17:23:25Z

OK, LGTM and we will track #262 separately.

rmjarvis changed the title ~~Setup py tests~~ Update unit tests for the new code installed via setup.py Nov 7, 2021

rmjarvis added 12 commits November 11, 2021 13:56

Update simple tests that basically already work

5a4af93

Add get_logger

6a2cbf3

Add FWHMgeom and FWHMeff to OpsimMetaDict

0029380

Update cosmic_rays tests

21438e7

Update PSF tests

2751280

Update treering tests

9cc000c

Update trimmer tests

7739ec3

Cache get_camera returns to avoid multiple loads of the LsstCam object

7839c76

Update instcat tests

f71562d

Add seds needed for tests in local test_sed_library directory

79dc5cf

Make object_is_valid check optional

962b26b

Fix psf tests that I apparently only half finished

f39de93

rmjarvis force-pushed the setup_py_tests branch from 9e58487 to f39de93 Compare November 11, 2021 19:06

rmjarvis added 7 commits November 11, 2021 14:39

Don't use LsstCamMapper in batoid unit tests

baba218

First try at ci.yml

acf11a2

forgot pipe

5bc4993

Add .condarc file

879f5ee

add conda_requirements.txt

ffd453b

Use bash -l {0} for shell

4e4f6e6

Switch to mamba for faster build

32759b8

rmjarvis added 2 commits November 12, 2021 11:24

Minor typos

2ceb4f8

Switch to pip install . and pytest to be more modern

462176c

jchiang87 requested changes Nov 19, 2021

View reviewed changes

rmjarvis added 4 commits November 19, 2021 15:04

Remove test_logging.py as no longer relevant

704f4a1

Remove test_config_reader.py

3ad40e7

Remove test_TracebackDecorator.py

2b82b10

Resolve question about TAI time and put note in doc string.

ca50b23

rmjarvis added 4 commits November 19, 2021 15:33

Treat dust=none as Av=0, Rv=1, not skipped. Closes #213

4374bc6

Remove chunking test

f61f35f

Remove extra_commands stuff in tests

0c559a1

Resolve comment about nsnap=2

9a0fbc0

jchiang87 requested changes Nov 22, 2021

View reviewed changes

Exclude Latiss from valid cameras

0dcabea

jchiang87 approved these changes Nov 22, 2021

View reviewed changes

Add tests of actual separation in spherical geom in WCS tests

fe4cac6

rmjarvis merged commit 3f304bd into main Nov 23, 2021

rmjarvis deleted the setup_py_tests branch November 23, 2021 18:25

rmjarvis restored the setup_py_tests branch November 23, 2021 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update unit tests for the new code installed via setup.py #259

Update unit tests for the new code installed via setup.py #259

rmjarvis commented Nov 5, 2021

rmjarvis commented Nov 12, 2021

cwwalter commented Nov 12, 2021

jchiang87 left a comment

jchiang87 Nov 19, 2021

jchiang87 Nov 19, 2021

rmjarvis Nov 19, 2021

jchiang87 Nov 19, 2021

jchiang87 Nov 19, 2021

rmjarvis Nov 19, 2021

jchiang87 Nov 19, 2021 •

edited

Loading

cwwalter Nov 19, 2021

rmjarvis Nov 19, 2021

jchiang87 Nov 19, 2021

rmjarvis Nov 19, 2021 •

edited

Loading

jchiang87 Nov 19, 2021

rmjarvis Nov 19, 2021

jchiang87 Nov 19, 2021

rmjarvis Nov 19, 2021

jmeyers314 Nov 19, 2021

rmjarvis Nov 19, 2021

cwwalter Nov 21, 2021

jchiang87 Nov 19, 2021

jchiang87 Nov 19, 2021

jchiang87 Nov 19, 2021

jchiang87 Nov 19, 2021

jchiang87 left a comment

jchiang87 Nov 22, 2021

rmjarvis Nov 22, 2021

rmjarvis Nov 22, 2021

cwwalter Nov 22, 2021

jmeyers314 Nov 22, 2021

rmjarvis commented Nov 22, 2021

cwwalter commented Nov 23, 2021


		# XXX: I think this is probably not needed anymore. Now use GalSim logger.

Update unit tests for the new code installed via setup.py #259

Update unit tests for the new code installed via setup.py #259

Conversation

rmjarvis commented Nov 5, 2021

rmjarvis commented Nov 12, 2021

cwwalter commented Nov 12, 2021

jchiang87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchiang87 Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmjarvis Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchiang87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmjarvis commented Nov 22, 2021

cwwalter commented Nov 23, 2021

jchiang87 Nov 19, 2021 •

edited

Loading

rmjarvis Nov 19, 2021 •

edited

Loading