search

AstroHuntsman / gunagala

4 stars 9 forks source link

Image simulation #16

Closed AnthonyHorton closed 4 years ago

AnthonyHorton commented 6 years ago

Adds ability to generated simulated image data to the Imager class, based on old astroimsim code.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-2.9%) to 87.656% when pulling 04fab438bbddcb6d8f35986afb4ce0ec7b9dfc75 on image_sim into 6e776061020309e0ba394371b6a3ea02bbd53341 on develop.

lspitler commented 6 years ago

@AnthonyHorton let me know if you'd like help on this. I can try to move this forward if you let me know what it needs or if you'd just like a review.

AnthonyHorton commented 6 years ago

@lspitler Main Issue preventing merging was excessive memory use with the analytic PSFs. The pixel based ones I already sorted out, but something similar needs to be done when using the other type.

lspitler commented 6 years ago

@AnthonyHorton I guess you want to write tests too? Under the new short PR regime, would it make sense to merge & close and define the remaining problem in a new Issue? Let me know if you think I can do any of this for you.

AnthonyHorton commented 6 years ago

@lspitler Yes, the image simulation stuff doesn't have any tests, hence the -3% coverage change. Not too happy with that, either.

Somewhat uncomfortable with merging this into develop while it still has code that it likely to bring your computer to a grinding halt if you try to use it (it's really that bad!). I guess what we could do though is to temporarily disable the image simulation for the analytic PSFs (i.e. insert a check at the start of the make_noiseless_image method that raises an informative NotImplementedErrror if you try to use it with the analytic PSFs) and write up an Issue to come back and fix the memory usage later. Then we could merge this branch and get our feature branches based on develop again, as they should be.

lspitler commented 6 years ago

@AnthonyHorton that sounds like a great plan! Write up the Issue if you can and I'll try and tackle.

lspitler commented 4 years ago

@AnthonyHorton OK I implemented a NotImplementedError solution to the analytical PSF issue as suggested here: https://github.com/AstroHuntsman/gunagala/pull/16/#issuecomment-426844974

You already created an Issue to fix it someday: #29

Can you please do quick review of the changes here so we can merge this into develop before the X-sensing Hack begins tomorrow?