dirac-institute / sorcha

An open-source community LSST Solar System Simulator
Other
16 stars 17 forks source link

Two unit tests fail if sorcha-addons is installed #995

Closed astronomerritt closed 1 month ago

astronomerritt commented 2 months ago

Two of Sorcha's unit tests fail if sorcha-addons is installed.

First, in tests/activity/test_activity_registration.py, test_register_subclasses() fails with an assertion error. When I print the variables in question:

>> print(output)

{'identity': <class 'sorcha.activity.identity_activity.IdentityCometaryActivity'>, 'lsst_comet': <class 'sorcha_addons.activity.lsst_comet.lsst_comet_activity.LSSTCometActivity'>}

>> print (CA_METHODS)

{'identity': <class 'sorcha.activity.identity_activity.IdentityCometaryActivity'>}

Second, in tests/lightcurves/test_lightcurve_registration.py, test_register_subclasses() fails similarly.

>> print(output)

{'identity': <class 'sorcha.lightcurves.identity_lightcurve.IdentityLightCurve'>, 'sinusoidal': <class 'sorcha_addons.lightcurve.sinusoidal.sinusoidal_lightcurve.SinusoidalLightCurve'>}

>> print (CA_METHODS)

{'identity': <class 'sorcha.lightcurves.identity_lightcurve.IdentityLightCurve'>}

Looking at this, I think I can see how desired behaviour is accidentally tripping the unit tests. I'd still like @bernardinelli to look at it please :)

drewoldag commented 2 months ago

I believe what is happening here is that when CA_METHODS is initialized, the python interpreter hasn't found the sorcha_addons yet, and so those classes aren't included in the CA_METHODS dictionary.

To ensure that we're picking up all the sorcha_addons, we can call either update_activity_subclasses or update_lc_subclasses. We have to do a similar thing in sorcha.py here: https://github.com/dirac-institute/sorcha/blob/main/src/sorcha/sorcha.py#L104-L105

My recommendation would be to add update_activity_subclasses() on line 12 of test_activity_registration.py and update_lc_subclasses() on line 12 of test_lightcurve_registration.py.

I believe that will correctly update the CA_METHODS and LA_METHODS in the same way that we do in the normal use of Sorcha.

mschwamb commented 1 month ago

@astronomerritt can you put the fix for this into it's own PR since the chunking PR might not be merged for a long while