Skip to content

Tickets/DM-54855#1324

Open
natelust wants to merge 2 commits into
mainfrom
tickets/DM-54855
Open

Tickets/DM-54855#1324
natelust wants to merge 2 commits into
mainfrom
tickets/DM-54855

Conversation

@natelust

Copy link
Copy Markdown
Contributor

No description provided.

natelust added 2 commits June 18, 2026 15:16
Create a series of tasks that mimic the rgb2hips code which make
hips trees in parallel. Unlike the rgb code, this produces fits
files that are not compressed or processed other that what is
needed to create tiles.

@fred3m fred3m left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few bugs the need to be corrected. Maybe add a few unit tests to check that everything works correctly, they likely would have caught at least the indexing issue.


image = handle.get()
mosaic_maker.add_to_image(new_array, image.image.array, tmp_new_box, tmpBox, reverse=False)
boxes.append((new_array, skyWcs, new_box))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comparing to rgb2hips and looking at the logic, I think that this line is indented one too many times and should be outside of the above loop

for j in range(binned_array.shape[1] // 512):
pixel_id = int(hpx_id_array[i, j])
sub_pixel = binned_array[i * 512 : i * 512 + 512, j * 512 : j * 512 + 512]
self.log.info(f"writing sub_pixel {pixel_id}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should use lazy formatting, as you used above

- "float": 32-bit floating-point numbers
tile_wcs : `~lsst.afw.geom.SkyWcs` or `None`
If supplied, this wcs is written to fits images.
Raises

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blank line above

Comment on lines +413 to +415
allsky_image = np.zeros(
(n_tiles_wide * tile_size, n_tiles_high * tile_size),
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that you need to switch the first two shape dimensions. The above worked in rgb2hips because it was a PIL Image, but here it is an ndarray.

(column + 1) * tile_size,
(row + 1) * tile_size,
)
tile_image_shrunk = tile_image.resize((tile_size, tile_size))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that ndarray.resize is in place (see docs.) You probably need to use tile_image_shrunk = np.resize(tile_image, (tile_size, tile_size))

Comment on lines +264 to +265
bitpix = 32
hbitpix = 32

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per FITS guidelines BITPIX should be negative for floating point data: https://archive.stsci.edu/fits/fits_standard/node39.html#s:man

Comment on lines +317 to +326
if band in properties_config.spectral_ranges:
em_min = properties_config.spectral_ranges[band].lambda_min / 1e9
else:
self.log.warning("blue band %s not in self.config.spectral_ranges.", band)
em_min = 3e-7
if band in properties_config.spectral_ranges:
em_max = properties_config.spectral_ranges[band].lambda_max / 1e9
else:
self.log.warning("red band %s not in self.config.spectral_ranges.", band)
em_max = 1e-6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You have two duplicate checks, since for a single band fits there is a single band config property as opposed to bluest_band and reddest_band in rgb2hips. You should set em_min and em_max in the same block and if the band isn't contained, you should probably raise, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants