DES-SL / BlueRings

An extended re-implementation of Gavazzi et al's RingFinder galaxy-scale strong gravitational lens detection robot. Subtract red light from blue, analyze residuals, classify.
MIT License
1 stars 3 forks source link

Refactoring to be able to use DES tiles #16

Open tcollett opened 8 years ago

tcollett commented 8 years ago

@drphilmarshall I have more to come, but would you mind reviewing my code when I'm ready please? Thanks!

drphilmarshall commented 8 years ago

Hey @tcollett ! Did you manage to set up your laptop to be able to push changes to GitHub? If so, please do go ahead! We have work to do on the DES tiles :-)

tcollett commented 8 years ago

I'm still having issues with the pushing and pulling. I think I've now broken everything.

But the refactoring is finished. Shall I email you the new code ;)

tcollett commented 8 years ago

Ok, after much 'fun', I think I've solved the pushing problem with this branch. The pushed version seems to be up to date and refactored.

drphilmarshall commented 8 years ago

Great, thanks @tcollett ! I pulled from your branch and tried to run the demo notebook, and it broke with the error below - you should fix the demo notebook so it runs. Then, would it be easy for your to check in a quick notebook showing the code working on a DES tile? That would be a useful check that the code was working, and would show Nikolay what is going on.


TypeError Traceback (most recent call last)

in () 21 22 psfmode="dont" ---> 23 RF=RingFinder(imgg,imgi,sigg,sigi,psfg,psfi,0.265,1e12,1e12,visualize=False,psfmode=psfmode) 24 RFgz=RingFinder(imgg,imgz,sigg,sigi,psfg,psfz,0.265,1e12,1e12,visualize=False,psfmode=psfmode) 25 RFrz=RingFinder(imgr,imgz,sigr,sigz,psfr,psfz,0.265,1e12,1e12,visualize=False,psfmode=psfmode) /Users/pjm/work/stronglensing/DES/BlueRings/StandAloneRingFinder.pyc in **init**(self, B, R, sB, sR, pB, pR, pixelsize, zerofluxB, zerofluxR, visualize, psfmode) 20 21 ---> 22 comR=getPeak(R,xc,yc) 23 24 """ /Users/pjm/work/stronglensing/DES/BlueRings/StandAloneRingFinder.pyc in getPeak(img, x, y) 274 d = ndimage.gaussian_filter(img,1.) 275 Y,X = numpy.mgrid[0:d.shape[0],0:d.shape[1]] --> 276 Y -= Y.mean() 277 X -= X.mean() 278 d /= d.sum() TypeError: Cannot cast ufunc subtract output from dtype('float64') to dtype('int64') with casting rule 'same_kind'
drphilmarshall commented 8 years ago

Most of the code in the RingFindforTile class seems to be for setting up a data structure based on a FITS file tile, so I think this class should be called Tile. This fits with the Cutout object idea in my other comments. In this scheme, a Tile object would know how to make, subtract and present cutouts of itself. The calling script would be something like:

tile = bluerings.Tile(`des_tile_0001.fits`)
tile.make_cutouts(objectIDs)
tile.make_rgb_composite_images(type='unsubtracted')
tile.run_ringfinder()
tile.make_rgb_composite_images(type='subtracted')
tile.zip_up_pngs()

The make_cutouts method would contain lines like

self.cutouts[k] = bluerings.Cutout(objectIDs[k])

and the run_ringfinder method would contain lines like

self.subtractions[k] = bluerings.RingFinder(self.cutouts[k])

What do you think? BlueRings Cutout and Tile objects would be pretty re-useable, by the convnet groups for example.

tcollett commented 8 years ago

These are all good ideas that I agree with in principle, but I'm not sure how much this tile format is fixed - it may just be a one off from nikolay, which is why I didn't refactor the tile reading code.

On 2 August 2016 at 18:19, Phil Marshall notifications@github.com wrote:

Most of the code in the RingFindforTile class seems to be for setting up a data structure based on a FITS file tile, so I think this class should be called Tile. This fits with the Cutout object idea in my other comments. In this scheme, a Tile object would know how to make, subtract and present cutouts of itself. The calling script would be something like:

tile = bluerings.Tile(des_tile_0001.fits) tile.make_cutouts(objectIDs) tile.make_rgb_composite_images(type='unsubtracted') tile.run_ringfinder() tile.make_rgb_composite_images(type='subtracted') tile.zip_up_pngs()

The make_cutouts method would contain lines like

self.cutouts[k] = bluerings.Cutout(objectIDs[k])

and the run_ringfinder method would contain lines like

self.subtractions[k] = bluerings.RingFinder(self.cutouts[k])

What do you think? BlueRings Cutout and Tile objects would be pretty re-useable, by the convnet groups for example.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-236975897, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc1Bs00QnEPGySdGQeBQ3OEKv99OcvDks5qb3wygaJpZM4JOxIw .

drphilmarshall commented 8 years ago

Good point. However, the input images are always going to be provided as some sort of blob, which we need to read in and work with. If we refactored to work with a "Tile" object, and then later we have to switch to data provided in a folder or a tarball or a database or whatever, we could abstract the common parts of the "Tile" class into a more general "Blob" class and then have both "Tile" and "Folder"/"Tarball"/"Database" inherit from "Blob". That way we would not have to re-write the RF calls (which would still be handled by the run_ringfinder method in Blob.

On Tue, Aug 2, 2016 at 11:50 AM, Thomas Collett notifications@github.com wrote:

These are all good ideas that I agree with in principle, but I'm not sure how much this tile format is fixed - it may just be a one off from nikolay, which is why I didn't refactor the tile reading code.

On 2 August 2016 at 18:19, Phil Marshall notifications@github.com wrote:

Most of the code in the RingFindforTile class seems to be for setting up a data structure based on a FITS file tile, so I think this class should be called Tile. This fits with the Cutout object idea in my other comments. In this scheme, a Tile object would know how to make, subtract and present cutouts of itself. The calling script would be something like:

tile = bluerings.Tile(des_tile_0001.fits) tile.make_cutouts(objectIDs) tile.make_rgb_composite_images(type='unsubtracted') tile.run_ringfinder() tile.make_rgb_composite_images(type='subtracted') tile.zip_up_pngs()

The make_cutouts method would contain lines like

self.cutouts[k] = bluerings.Cutout(objectIDs[k])

and the run_ringfinder method would contain lines like

self.subtractions[k] = bluerings.RingFinder(self.cutouts[k])

What do you think? BlueRings Cutout and Tile objects would be pretty re-useable, by the convnet groups for example.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-236975897, or mute the thread < https://github.com/notifications/unsubscribe-auth/ABc1Bs00QnEPGySdGQeBQ3OEKv99OcvDks5qb3wygaJpZM4JOxIw

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-237004184, or mute the thread https://github.com/notifications/unsubscribe-auth/AArY90B6xpesWxwSzcIKluY5s0m1SGeMks5qb5FwgaJpZM4JOxIw .

tcollett commented 8 years ago

Yeah that's a nice option. A blob class that runs BlueRings on the data and is inherited by all data readers is nice.

The most important thing is the BlueRings is a separate class to the code that reads in the data - then we can release BlueRings publicly for any survey to use.

On 2 August 2016 at 20:28, Phil Marshall notifications@github.com wrote:

Good point. However, the input images are always going to be provided as some sort of blob, which we need to read in and work with. If we refactored to work with a "Tile" object, and then later we have to switch to data provided in a folder or a tarball or a database or whatever, we could abstract the common parts of the "Tile" class into a more general "Blob" class and then have both "Tile" and "Folder"/"Tarball"/"Database" inherit from "Blob". That way we would not have to re-write the RF calls (which would still be handled by the run_ringfinder method in Blob.

On Tue, Aug 2, 2016 at 11:50 AM, Thomas Collett notifications@github.com wrote:

These are all good ideas that I agree with in principle, but I'm not sure how much this tile format is fixed - it may just be a one off from nikolay, which is why I didn't refactor the tile reading code.

On 2 August 2016 at 18:19, Phil Marshall notifications@github.com wrote:

Most of the code in the RingFindforTile class seems to be for setting up a data structure based on a FITS file tile, so I think this class should be called Tile. This fits with the Cutout object idea in my other comments. In this scheme, a Tile object would know how to make, subtract and present cutouts of itself. The calling script would be something like:

tile = bluerings.Tile(des_tile_0001.fits) tile.make_cutouts(objectIDs) tile.make_rgb_composite_images(type='unsubtracted') tile.run_ringfinder() tile.make_rgb_composite_images(type='subtracted') tile.zip_up_pngs()

The make_cutouts method would contain lines like

self.cutouts[k] = bluerings.Cutout(objectIDs[k])

and the run_ringfinder method would contain lines like

self.subtractions[k] = bluerings.RingFinder(self.cutouts[k])

What do you think? BlueRings Cutout and Tile objects would be pretty re-useable, by the convnet groups for example.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-236975897, or mute the thread <

https://github.com/notifications/unsubscribe-auth/ABc1Bs00QnEPGySdGQeBQ3OEKv99OcvDks5qb3wygaJpZM4JOxIw

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-237004184, or mute the thread < https://github.com/notifications/unsubscribe-auth/AArY90B6xpesWxwSzcIKluY5s0m1SGeMks5qb5FwgaJpZM4JOxIw

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-237016243, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc1BnxsWmwmGr0W16WXUMDIoJb5A3tuks5qb5pxgaJpZM4JOxIw .

drphilmarshall commented 8 years ago

OK, sounds good. Do you want to give it a go refactoring into a Tile class and a Cutout class as part of this pull request, or merge this one first (as soon as the demo[s] work[s])?

tcollett commented 8 years ago

I think these are separate. I also wouldn't merge this yet, since BlueRings needs a signal-to-noise cut too. (I think I have it working and correct but there is something funky which I think is related to the sigmas that Nikolay generated that needs some exploring.)

On 2 August 2016 at 22:55, Phil Marshall notifications@github.com wrote:

OK, sounds good. Do you want to give it a go refactoring into a Tile class and a Cutout class as part of this pull request, or merge this one first (as soon as the demo[s] work[s])?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-237057736, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc1Bl-IWFtBq_Xq4HYW1wzXd3GpCYF2ks5qb7y2gaJpZM4JOxIw .

tcollett commented 8 years ago

Yes there's something funky with the noise maps, certainly the z band ones are wrong. I've pinged @bnord for a solution.

I'm pretty sure the code is right though, for getting the Signal-to-noise of all residuals. We may need to do either some kind of object identification, or only sum residuals over a centred annulus, but hopefully we don't have to.

drphilmarshall commented 8 years ago

@tcollett I see you have the S/N threshold set to 0.2 - did you mean 5 instead?

Interested to hear how this filtering is working out - Do you think we need to go to estimating S/N in an annulus, to avoid PSF issues in the central region and neighboring galaxies further out?

I would think that the residuals in the other bands besides g could be useful, especially for red sources - combined S/N in all filters should be easy, no? I wonder if you could use the extracted values to construct a simple "SED" and ask for it to be astronomical in shape, roughly - we're not interested in residuals that are only bright in r and not i, for example.

On Wed, Aug 17, 2016 at 4:22 AM, Thomas Collett notifications@github.com wrote:

Yes there's something funky with the noise maps, certainly the z band ones are wrong. I've pinged @bnord https://github.com/bnord for a solution.

I'm pretty sure the code is right though, for getting the Signal-to-noise of all residuals. We may need to do either some kind of object identification, or only sum residuals over a centred annulus, but hopefully we don't have to.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-240383862, or mute the thread https://github.com/notifications/unsubscribe-auth/AArY95WNoN74R11V7ZUdcepDHAppQh-0ks5qgu76gaJpZM4JOxIw .

tcollett commented 8 years ago

Ok, so I solved the noise level issues. No the 0.2 was because of those issues. I suspect we'll want something like 10 as the SN threshold in the end.

But, new issues have cropped up in how to optimally search for features in noisy data. Turns out you can't just add up all the positive pixels as lots of them are noise - obvious in hindsight, but more thought needed on how to do this. (pre-filter to smooth out noise?)

Yes the other bands can trivially be added, but we have to do some kind of object detection if we want to look at SEDs. (BTW can't high redshift starformation make a source that's bright in r but not i?)

On 22 August 2016 at 17:59, Phil Marshall notifications@github.com wrote:

@tcollett I see you have the S/N threshold set to 0.2 - did you mean 5 instead?

Interested to hear how this filtering is working out - Do you think we need to go to estimating S/N in an annulus, to avoid PSF issues in the central region and neighboring galaxies further out?

I would think that the residuals in the other bands besides g could be useful, especially for red sources - combined S/N in all filters should be easy, no? I wonder if you could use the extracted values to construct a simple "SED" and ask for it to be astronomical in shape, roughly - we're not interested in residuals that are only bright in r and not i, for example.

On Wed, Aug 17, 2016 at 4:22 AM, Thomas Collett notifications@github.com wrote:

Yes there's something funky with the noise maps, certainly the z band ones are wrong. I've pinged @bnord https://github.com/bnord for a solution.

I'm pretty sure the code is right though, for getting the Signal-to-noise of all residuals. We may need to do either some kind of object identification, or only sum residuals over a centred annulus, but hopefully we don't have to.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-240383862, or mute the thread https://github.com/notifications/unsubscribe-auth/ AArY95WNoN74R11V7ZUdcepDHAppQh-0ks5qgu76gaJpZM4JOxIw .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-241478764, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc1Bl1nYVYTdeM4wdFGTwuINARD5asNks5qidWLgaJpZM4JOxIw .

drphilmarshall commented 8 years ago

Smoothing sounds sensible. You could also sum the negative pixels as well, so they cancel the positive noise values (only on average, though).

I don't think you need to do object detection to claim an SED - but I agree that if you have multiple objects in the flux measurement region the interpretation of teh SED could be tricky.

On Mon, Aug 22, 2016 at 10:08 AM, Thomas Collett notifications@github.com wrote:

Ok, so I solved the noise level issues. No the 0.2 was because of those issues. I suspect we'll want something like 10 as the SN threshold in the end.

But, new issues have cropped up in how to optimally search for features in noisy data. Turns out you can't just add up all the positive pixels as lots of them are noise - obvious in hindsight, but more thought needed on how to do this. (pre-filter to smooth out noise?)

Yes the other bands can trivially be added, but we have to do some kind of object detection if we want to look at SEDs. (BTW can't high redshift starformation make a source that's bright in r but not i?)

On 22 August 2016 at 17:59, Phil Marshall notifications@github.com wrote:

@tcollett I see you have the S/N threshold set to 0.2 - did you mean 5 instead?

Interested to hear how this filtering is working out - Do you think we need to go to estimating S/N in an annulus, to avoid PSF issues in the central region and neighboring galaxies further out?

I would think that the residuals in the other bands besides g could be useful, especially for red sources - combined S/N in all filters should be easy, no? I wonder if you could use the extracted values to construct a simple "SED" and ask for it to be astronomical in shape, roughly - we're not interested in residuals that are only bright in r and not i, for example.

On Wed, Aug 17, 2016 at 4:22 AM, Thomas Collett < notifications@github.com> wrote:

Yes there's something funky with the noise maps, certainly the z band ones are wrong. I've pinged @bnord https://github.com/bnord for a solution.

I'm pretty sure the code is right though, for getting the Signal-to-noise of all residuals. We may need to do either some kind of object identification, or only sum residuals over a centred annulus, but hopefully we don't have to.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-240383862, or mute the thread https://github.com/notifications/unsubscribe-auth/ AArY95WNoN74R11V7ZUdcepDHAppQh-0ks5qgu76gaJpZM4JOxIw .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-241478764, or mute the thread https://github.com/notifications/unsubscribe-auth/ ABc1Bl1nYVYTdeM4wdFGTwuINARD5asNks5qidWLgaJpZM4JOxIw .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DES-SL/BlueRings/pull/16#issuecomment-241481199, or mute the thread https://github.com/notifications/unsubscribe-auth/AArY9zU00P0jEn-65jCSiOsZZ5mz9yYiks5qideOgaJpZM4JOxIw .