Skip to content

Diffsky26 updates - #116

Merged
arunkannawadi merged 6 commits into
mainfrom
diffsky26_updates
Nov 4, 2026
Merged

arunkannawadi merged 6 commits into
mainfrom
diffsky26_updates

Conversation

@matroxel

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread roman_imsim/sed_prefetch.py Outdated
)
diffsky_objects[0].prefetch_seds(diffsky_objects)

skycat._prefetched_sed_batch = batch_key

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This entire file looks like a duplicate of changes made in skycat.py. Should we remove this and just use the function defined there?

Comment thread roman_imsim/skycat.py Outdated
"""
return self.getNObjects()

def prefetch_seds(skycat, start, stop):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is correct syntax, but not referring to the first argument of this method as self can be jarring to read. This is probably the result of copying it from sed_prefetch.py.

Comment thread roman_imsim/skycat.py Outdated
# and only once for the range.
sed_batch = base.get("_roman_imsim_sed_batch")
if sed_batch is not None:
skycat.prefetch_seds(*sed_batch)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
skycat.prefetch_seds(*sed_batch)
start, stop = sed_batch
skycat.prefetch_seds(start=start, stop=stop)

@matroxel

Copy link
Copy Markdown
Collaborator Author

Forgot to remove the extra file. I think these should be addressed now in new push.

Copilot AI 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.

🟡 Changes recommended

Prefetching confuses stamp numbers with catalog indices, causing valid repeated-index configurations to fail.

1 open issue
What changed in this PR

Adds Diffsky26 support to sky-catalog rendering with batch-based runtime spectral prefetching.

Changes:

  • Recognizes Diffsky26 galaxies and prefetches their spectra.
  • Makes object batch size configurable and passes batch bounds through both image builders.
  • Adds tests for prefetch selection, caching, and bounds validation.
File Description
tests/​test_skycat_prefetch.py Tests prefetch behavior and validation.
roman_imsim/​skycat.py Adds Diffsky26 classification and spectral prefetching.
roman_imsim/​sca.py Configures batch sizes and exposes rendering ranges.

Review effort: Balanced

Comment thread roman_imsim/skycat.py Resolved

@arunkannawadi arunkannawadi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These changes don't hamper the end-to-end tests: https://github.com/DukeCosmology/roman_imsim/actions/runs/39614542590

@arunkannawadi

Copy link
Copy Markdown
Member

I tried running another end-to-end test just to be sure, but building esutil is failing now. Since I tested it earlier and we don't expect this to change the current behavior, I'm merging this right away.

@arunkannawadi
arunkannawadi merged commit 4ef4182 into main Nov 4, 2026
7 of 8 checks passed
@arunkannawadi
arunkannawadi deleted the diffsky26_updates branch November 4, 2026 17:58
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.

4 participants