Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have Standardizers add obstime to LayeredImage #525

Merged
merged 8 commits into from
Mar 21, 2024
Merged

Have Standardizers add obstime to LayeredImage #525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aee11a5
Have fits and butler standardizers set obstimes
wilsonbb Mar 7, 2024
0c62513
Clean up unit tests
wilsonbb Mar 18, 2024
db14da7
Merge branch 'main' into standard
wilsonbb Mar 18, 2024
30ced4c
Formatting fixes
wilsonbb Mar 19, 2024
da1a2a8
Fix comments
wilsonbb Mar 19, 2024
3af3753
Address comments
wilsonbb Mar 19, 2024
f51d4c6
Add type conversion back in for consistency
wilsonbb Mar 19, 2024
a3c9abf
Merge branch 'main' into standard
wilsonbb Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/kbmod/standardizers/butler_standardizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,13 @@ def toLayeredImage(self):
psfs = self.standardizePSF()

# guaranteed to exist, i.e. safe to access
mjds = meta["mjd"]
if isinstance(meta["mjd"], (list, tuple)):
mjds = meta["mjd"]
else:
mjds = (meta["mjd"] for e in self.processable)

imgs = []
for sci, var, mask, psf, t in zip(sciences, variances, masks, psfs, mjds):
imgs.append(LayeredImage(RawImage(sci), RawImage(var), RawImage(mask), t, psf))
mask = mask.astype(np.float32)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we explicitly casting the mask to float32? Is Eigen complaining to you? If so then we should address this in the standardizeMaskImage method.

Copy link
Collaborator Author

@wilsonbb wilsonbb Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added this because we were doing it in the FITS standardizer and I thought that it might help to catch some edge case.

In ButlerStandardizer.standardizeMaskImage() we do currently explicitly convert to a bool dtype. Do you know if there was any particular reason why we preferred the bool over float32 dtype? Presumably to save on memory but that adds confusion as to why we cast the mask to float32 in other standardizers

Copy link
Collaborator Author

@wilsonbb wilsonbb Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though actually taking another look, in main we do still still get back to a float32, probably in the conversion to Eigen when we create the RawImage

I'm starting to feel that we should remove all the type conversions both in here and instandardizeMaskImage

Uploading Screenshot 2024-03-19 at 3.08.51 PM.png…

imgs.append(LayeredImage(RawImage(sci, t), RawImage(var, t), RawImage(mask, t), psf))
return imgs
5 changes: 3 additions & 2 deletions src/kbmod/standardizers/fits_standardizers/fits_standardizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ def toLayeredImage(self):
# copy. TODO: fix when/if CPP stuff is fixed.
imgs = []
for sci, var, mask, psf, t in zip(sciences, variances, masks, psfs, mjds):
imgs.append(LayeredImage(RawImage(sci), RawImage(var), RawImage(mask.astype(np.float32)), psf))

# Converts nd array mask from bool to np.float32
mask = mask.astype(np.float32)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What dtype does Rubin butler return for the mask? I have a feeling I assumed something here and then forgot about it. Mostly astropy.io.fits and the butler should already return float32s. I think Butler might be returning an int for mask, but in that case this cast would still be better in the standardizer I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This comment seems to be about the butler standardizer but is on the FITS standardizer file? Sorry if I'm the one confused here, but answering about the FITS standardizers)

Yeah I saw that you added it in this "Fix bug in Fits Standardizers." commit but I am also not sure what it was you were specifically trying to fix. But overall the FITS standardizer seems fairly broad/general so it seems like it could be fine to do a default conversion of the dtype

Related, we also currently spoof that the mask should be np.int32 here and then in turn explicitly cast the mask dtype to be boolean ion KBMODV1._standardize_mask().

So I guess the overall the question maybe is just what do we want the mask dtype to be since we seem to be a bit inconsistent about it

Copy link
Member

@DinoBektesevic DinoBektesevic Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array is read into the memory by Astropy based on the header keywords DTYPE. It's the same for Rubin. Originally Eigen proposal was to have 3 different arrays (float, double and ints) but that was shut down. This induces a copy and a cast to float, since float is the only array people wanted. There's really no need to have more than int8 for the mask and it would save space on the GPU, but the work to get that done was deemed too much to do at the time.

So this is true for both Rubin and any of the FITS standardizers. Then I floated the casts up to Python by banning the implicit conversion and casts in the Python bindings in C++:

https://github.com/dirac-institute/kbmod/blob/main/src/kbmod/search/raw_image.cpp#L465

But I seem to have a few unnecessary ones. From the code it seems like I wanted to add a constructor onto LayeredImage that takes a single t and propagates it down, but I seem to have forgot or something. Rubin seems to return int masks, other files seems to return whatever they want. This can be fixed and reduce memory and time requirements, but it's nothing that should be addressed in this PR so it's all ok. Just one more thing for the TODO list.

imgs.append(LayeredImage(RawImage(sci, t), RawImage(var, t), RawImage(mask, t), psf))
return imgs
26 changes: 26 additions & 0 deletions tests/test_butlerstd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,32 @@ def test_psf(self):
self.assertIsInstance(psf, PSF)
self.assertEqual(psf.get_std(), std.config["psf_std"])

def test_to_layered_image(self):
"""Test ButlerStandardizer can create a LayeredImage."""
std = Standardizer.get(DatasetId(8), butler=self.butler)
self.assertIsInstance(std, ButlerStandardizer)

# Get the expected FITS files and extract the MJD from the header
fits = FitsFactory.get_fits(8, spoof_data=True)
hdr = fits["PRIMARY"].header
expected_mjd = Time(hdr["DATE-AVG"]).mjd

# Get list of layered images froom the standardizer
butler_imgs = std.toLayeredImage()
self.assertEqual(1, len(butler_imgs))
img = butler_imgs[0]

# Compare standardized images
np.testing.assert_equal(fits["IMAGE"].data, img.get_science().image)
np.testing.assert_equal(fits["VARIANCE"].data, img.get_variance().image)
np.testing.assert_equal(fits["MASK"].data, img.get_mask().image)

# Test that we correctly set metadata
self.assertEqual(expected_mjd, img.get_obstime())
self.assertEqual(expected_mjd, img.get_science().obstime)
self.assertEqual(expected_mjd, img.get_variance().obstime)
self.assertEqual(expected_mjd, img.get_mask().obstime)


if __name__ == "__main__":
unittest.main()
27 changes: 25 additions & 2 deletions tests/test_standardizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
KBMODV1,
KBMODV1Config,
FitsStandardizer,
SingleExtensionFits,
MultiExtensionFits,
)


Expand Down Expand Up @@ -344,6 +342,31 @@ def test_psf(self):
psf = next(std2.standardizePSF())
self.assertEqual(psf.get_std(), std2.config["psf_std"][0])

def test_to_layered_image(self):
"""Test that KBMODV1 standardizer can create LayeredImages."""
std = Standardizer.get(self.fits, force=KBMODV1)
self.assertIsInstance(std, KBMODV1)

# Get the expected FITS files and extract the MJD from the header
hdr = self.fits["PRIMARY"].header
expected_mjd = Time(hdr["DATE-AVG"], format="isot").mjd

# Get list of layered images from the standardizer
layered_imgs = std.toLayeredImage()
self.assertEqual(1, len(layered_imgs))
img = layered_imgs[0]

# Compare standardized images
np.testing.assert_equal(self.fits["IMAGE"].data, img.get_science().image)
np.testing.assert_equal(self.fits["VARIANCE"].data, img.get_variance().image)
np.testing.assert_equal(self.fits["MASK"].data, img.get_mask().image)

# Test that we correctly set metadata
self.assertEqual(expected_mjd, img.get_obstime())
self.assertEqual(expected_mjd, img.get_science().obstime)
self.assertEqual(expected_mjd, img.get_variance().obstime)
self.assertEqual(expected_mjd, img.get_mask().obstime)


if __name__ == "__main__":
unittest.main()
Loading