ENH: Provide consistent colormap use for Alterra and TPV25 valves#33
ENH: Provide consistent colormap use for Alterra and TPV25 valves#33aylward wants to merge 1 commit intoProject-MONAI:mainfrom
Conversation
WalkthroughTwo VTK-to-USD conversion notebooks updated with revised colorization parameters: intensity ranges narrowed and cmap algorithm changed from "hot" to "jet" with sigmoid scaling enabled. Control flow unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the valve USD-generation experiment notebooks to apply a consistent stress/strain-to-color mapping (fixed intensity range + chosen colormap + sigmoid normalization) when post-processing simulation primvars into displayColor.
Changes:
- Set a fixed
intensity_range=(25, 200)when applying colormaps from stress/strain primvars. - Switch colormap selection to
"jet"for these valve conversions. - Enable
use_sigmoid_scale=Trueto apply sigmoid normalization before colormap lookup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.ipynb | Updates the post-processing colormap application parameters for TPV25 valve USD output. |
| experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.ipynb | Updates the post-processing colormap application parameters for Alterra valve USD output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| " cmap=\"hot\",\n", | ||
| " # use_sigmoid_scale=True,\n", | ||
| " intensity_range=(25, 200),\n", | ||
| " cmap=\"jet\",\n", |
There was a problem hiding this comment.
In this call, the colormap is hard-coded as "jet" even though the surrounding notebook uses/prints DEFAULT_COLORMAP (and the subsampled conversion later passes cmap=DEFAULT_COLORMAP). This makes the log message misleading and causes full vs subsampled exports to use different colormaps; consider passing cmap=DEFAULT_COLORMAP here (and/or updating DEFAULT_COLORMAP to "jet") so the notebook stays internally consistent.
| " cmap=\"jet\",\n", | |
| " cmap=DEFAULT_COLORMAP,\n", |
| " cmap=\"hot\",\n", | ||
| " # use_sigmoid_scale=True,\n", | ||
| " intensity_range=(25, 200),\n", | ||
| " cmap=\"jet\",\n", |
There was a problem hiding this comment.
This notebook defines/prints DEFAULT_COLORMAP earlier, but this call hard-codes cmap="jet". To avoid configuration drift and confusing output (e.g., reported colormap vs applied colormap), consider using the DEFAULT_COLORMAP variable here (or updating/removing the DEFAULT_COLORMAP setting so there is a single source of truth).
| " cmap=\"jet\",\n", | |
| " cmap=DEFAULT_COLORMAP,\n", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.ipynb (1)
388-390: Use a shared colormap preset instead of hardcoded literals in this call.This works, but these literals can drift from other TPV25 colorization paths in the same notebook. A single preset keeps the mapping policy truly consistent.
♻️ Suggested refactor
# Configuration: choose colormap for visualization DEFAULT_COLORMAP = "viridis" # matplotlib colormap name +VALVE_STRESS_COLORMAP = { + "intensity_range": (25, 200), + "cmap": "jet", + "use_sigmoid_scale": True, +} @@ usd_tools.apply_colormap_from_primvar( str(output_usd), mesh_path1, color_primvar, - intensity_range=(25, 200), - cmap="jet", - use_sigmoid_scale=True, + intensity_range=VALVE_STRESS_COLORMAP["intensity_range"], + cmap=VALVE_STRESS_COLORMAP["cmap"], + use_sigmoid_scale=VALVE_STRESS_COLORMAP["use_sigmoid_scale"], bind_vertex_color_material=True, ) @@ usd_tools.apply_colormap_from_primvar( str(output_usd_sub), mesh_path, color_primvar, - cmap=DEFAULT_COLORMAP, + intensity_range=VALVE_STRESS_COLORMAP["intensity_range"], + cmap=VALVE_STRESS_COLORMAP["cmap"], + use_sigmoid_scale=VALVE_STRESS_COLORMAP["use_sigmoid_scale"], bind_vertex_color_material=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.ipynb` around lines 388 - 390, Replace the hardcoded parameters intensity_range=(25, 200), cmap="jet", use_sigmoid_scale=True with a single shared colormap preset (e.g., TPV25_COLORMAP_PRESET or get_tpv25_colormap_preset()) and pass that preset into the call; update the call site that currently supplies intensity_range, cmap and use_sigmoid_scale to instead read those values from the preset so all TPV25 colorization paths share the same mapping policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.ipynb`:
- Around line 388-390: Replace the hardcoded parameters intensity_range=(25,
200), cmap="jet", use_sigmoid_scale=True with a single shared colormap preset
(e.g., TPV25_COLORMAP_PRESET or get_tpv25_colormap_preset()) and pass that
preset into the call; update the call site that currently supplies
intensity_range, cmap and use_sigmoid_scale to instead read those values from
the preset so all TPV25 colorization paths share the same mapping policy.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.ipynbexperiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.ipynb
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Set colormap, intensity range, and use of sigmoid transfer function when mapping stress measure from valve simulations to colormaps when generating USD files.
Summary by CodeRabbit