Skip to content

universal lut cleanup#921

Open
pgbrodrick wants to merge 9 commits into
isofit:devfrom
pgbrodrick:uni_lut_cleanup
Open

universal lut cleanup#921
pgbrodrick wants to merge 9 commits into
isofit:devfrom
pgbrodrick:uni_lut_cleanup

Conversation

@pgbrodrick

Copy link
Copy Markdown
Collaborator

Cleanup to support a large prebuilt lut grid:

  • template construction update to not pop interpolation points
  • config check modifications to allow gte/lte/interp inside lut_grid
  • update radiative_transfer lut_grid to always pull from file (with adjustment) if prebuilt lut grid is used

@jammont jammont left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, just some comments on the config checks but otherwise looks to resolve the prebuilt-lut issue with apply_oe. Thanks!

Comment thread isofit/configs/sections/radiative_transfer_config.py Outdated
)
if np.unique(item).size < len(item):
errors.append(f"Detected duplicate values in lut_grid item {key}")
if "interp" not in item:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

interp shouldn't be a key of lut_grid, iirc. lut_grid is just that, the lut grid, while lut_names is the lut loading strategy, eg. lut_names: {dim: {"interp": val}, ...}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@jammont , I think you're off by a level - you're description above of lut_names is right. But what you have labeled as 'dim' is 'item', and I'm checking if 'interp' is in item.

We're definitely hitting this, as it was thrown as an error.

@github-actions

github-actions Bot commented Mar 31, 2026

Copy link
Copy Markdown
📊 Generated results:

URL: isofit/isofit-test-results#19
SHA: 3c48f5e

Comment thread isofit/utils/template_construction.py Outdated
@pgbrodrick

Copy link
Copy Markdown
Collaborator Author

@evan-greenbrg - I haven't tested yet, but can you see if the logic flow above (with respect to using statevector priors) matches what you'd hope it would look like in template_construction.py?

@evan-greenbrg

Copy link
Copy Markdown
Collaborator

I like building the rt_statevector in a verbose manner. I think that fits with the sprit of how the config is put together.

A quick plug for the issue I was seeing when putting together #848, was that the get_lut_subset call returned a dict form incompatible with downstream calls. I haven't traced through to see the fix here, but just wanted to mention in case this addressed a non-overlapping issue.

@pgbrodrick

Copy link
Copy Markdown
Collaborator Author

Yeah - can't promise that every pathway is covered, but that error is what I've tried to address in the config.py and radiative_transfer_config.py updates.

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.

3 participants