Closed danielsf closed 6 years ago
danielsf
commented
6 years ago @rhiannonlynne Do you still have time to look at your half of this PR? It's okay if not. Most of what I did not your part is speed up code that was pretty well-tested. There are meaningful changes to the AGN variability model, but, having gone over MacLeod et al. (on which it is based), I think I am going to do some major work on that in a different PR, anyway, to make sure we are getting the physics right.
rhiannonlynne
commented
6 years ago Hi Scott,
Like you said - most of the changes of the things I was looking at were small cleanups, so no problem. I don't really any comments on how you implemented the new variability modeling with the cache -- it looks reasonable, and I don't have any meaningful additions there.
I was glad to see the addition of the sqlalchemy 'text' method to the sqlconstraint :)
Everything looks ok to me.
Lynne
This pull request includes the code needed to generate simulated alerts according to Maria's avro schema. It also contains a lot of optimizations to the stellar variability model to get it to run faster.
I think the sensible thing to do is to split the review in two so Lynne will review changes to pre-existing code (since I already hit her with a review of a new module in sims_utils) and Bryce will review the new code. That means that your file-by-file responsibilities are
Bryce: python/.../utils/alertGenerator.py python/.../utils/avroAlertGenerator.py tests/test_alertGenerator.py tests/test_avroAlertGenerator.py examples/generate_stellar_alert_data.py examples/generate_agn_alert_data.py examples/generate_avro_alerts.py support_scripts/get_kepler_dmag.sh the contents of tests/testData/avroSchema/ (which I copied from the lsst-dm/sample-avro-alert repo)
Lynne: python/.../baseCatalogModels/BaseCatalogModels.py python/.../mixins/VariabilityMixin.py python/.../utils/CatalogTestUtils.py python.../utils/testUtils.py support_scripts/get_mdwarf_flares.sh tests/testDitherColumnDetection.py tests/testFastLightCurveGenerator.py tests/testLightCurveGenerator.py tests/testMLTflareModel.py tests/testParametrizedLightCurveMixin.py tests/testPhoSimCatalogs.py tests/testPhoSimVariability.py tests/testVariabilityMixins.py
I think this means you each get roughly the same number of lines of code (or, at least, similar amounts of aggravation) to review. I apologize if I am wrong.
Note: do not expect this branch to run until these PRs are also merged:
https://github.com/lsst/sims_utils/pull/32 https://github.com/lsst/sims_catalogs/pull/17
Also note: tomorrow (12/20) I am going to start staying home to help Serena dig out from under the pile of boxes that contains all of our worldly possessions, so don't feel rushed to complete this. I just wanted to issue this PR before the holidays so that I didn't come back after New Years and wonder what it was I had done. That being said: if you send me a review before New Years, I will respond to it before New Years. I want to be able to stop obsessing about this....
Thanks for doing this review.