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 2 commits intoProject-MONAI:mainfrom
Conversation
WalkthroughReworked two VTK→USD conversion notebooks to introduce a test-friendly quick_run mode, rename and reorganize path/config variables, centralize ConversionSettings and material data, subsample time-series for testing, unify conversion flow, and simplify post-processing colorization using primvar-driven colormaps with sigmoid scaling. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.ipynb`:
- Line 202: The section header "## 3. Convert TPV25" is incorrect; update the
notebook markdown cell that contains that exact string (the header "## 3.
Convert TPV25") to accurately describe the conversion being performed, e.g.
change it to "## 3. Convert Alterra" or "## 3. Convert Alterra valve to USD" so
the section title matches the Alterra conversion code and context.
- Around line 134-142: The code collects matches into alterra_series from
vtk_files using pattern but doesn't handle the case where no frames match;
before any access (e.g., before using alterra_series[0] later), check if
alterra_series is empty and raise a clear, dataset-specific exception (e.g.,
ValueError or a custom AlterraDatasetError) with a message that includes the
pattern and the source directory or vtk_files summary so callers can diagnose
missing frames/naming drift; perform this check immediately after
alterra_series.sort(...) and before downstream uses.
- Around line 96-99: The colormap configuration currently allows both "stress"
and "strain" in colormap_primvar_substrs which can silently pick strain and
misapply the fixed colormap_range_min/colormap_range_max; change
colormap_primvar_substrs to only ["stress"] and ensure downstream usage (where
colormap_name, colormap_range_min, colormap_range_max are applied) is based on
that stress-only list (or add a simple assertion that the selected primvar
equals "stress") so the fixed (25,200) range is only used for stress
visualizations.
In `@experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.ipynb`:
- Around line 258-263: The code pins vessel_path to a specific prim name
("TPV25Valve_object4"), which is brittle; instead detect the correct mesh prim
under the TPV25Valve meshes at runtime. Modify the logic around
conversion_settings.separate_objects_by_connectivity /
separate_objects_by_cell_type to enumerate children of
"/World/Meshes/TPV25Valve" (or the parent used), inspect prim names/attributes
or topology/point-count to find the intended mesh (e.g., the prim whose
topology/face-count or connectivity attribute matches the source VTK component),
and set vessel_path to that discovered prim path before the primvar lookup that
follows (the variable currently named vessel_path and the primvar query that
relies on it). Ensure fallback to the single mesh path when no separate objects
exist.
- Around line 132-140: The code collects matching VTK files into tpv25_series by
iterating over vtk_files with the regex pattern and then sorts it; add a guard
after tpv25_series.sort(...) that checks if tpv25_series is empty and raise a
FileNotFoundError with a clear message (including pattern and/or input directory
context) so later access like tpv25_series[0] fails fast and provides actionable
information. Ensure the check references the same variable name tpv25_series and
include enough context in the exception message to identify the missing TPV25
frames.
- Around line 94-97: The colormap source list colormap_primvar_substrs currently
allows both "stress" and "strain", which can let TPV25 use a different scalar
while still using the same colormap bounds
(colormap_range_min/colormap_range_max) and thus break the "consistent stress
colormap" requirement; update colormap_primvar_substrs to include only "stress"
so the notebook always uses stress as the scalar for colorization (leave
colormap_name and colormap_range_min/colormap_range_max as-is unless there’s a
separate range for strain to adjust).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa01d113-592b-4898-bd09-464f5e215eae
📒 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
| "colormap_primvar_substrs = [\"stress\", \"strain\"]\n", | ||
| "colormap_name = \"jet\" # matplotlib colormap name\n", | ||
| "colormap_range_min = 25\n", | ||
| "colormap_range_max = 200\n", |
There was a problem hiding this comment.
Restrict the colormap source to stress-only.
The PR is standardizing stress visualization, but ["stress", "strain"] lets this notebook silently switch to a different quantity while still applying the same fixed (25, 200) range. That makes Alterra/TPV25 outputs non-comparable whenever only a strain array matches.
Suggested change
-colormap_primvar_substrs = ["stress", "strain"]
+colormap_primvar_substrs = ["stress"]🤖 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_alterra_valve_to_usd.ipynb`
around lines 96 - 99, The colormap configuration currently allows both "stress"
and "strain" in colormap_primvar_substrs which can silently pick strain and
misapply the fixed colormap_range_min/colormap_range_max; change
colormap_primvar_substrs to only ["stress"] and ensure downstream usage (where
colormap_name, colormap_range_min, colormap_range_max are applied) is based on
that stress-only list (or add a simple assertion that the selected primvar
equals "stress") so the fixed (25,200) range is only used for stress
visualizations.
| "alterra_series = []\n", | ||
| "for vtk_file in vtk_files:\n", | ||
| " match = re.search(pattern, vtk_file.name)\n", | ||
| " if match:\n", | ||
| " time_step = int(match.group(1))\n", | ||
| " alterra_series.append((time_step, vtk_file))\n", | ||
| "\n", | ||
| "# Sort by time step\n", | ||
| "alterra_series.sort(key=lambda x: x[0])" |
There was a problem hiding this comment.
Fail fast when no Alterra frames are found.
If the regex filter yields no matches, the notebook reaches alterra_series[0] on Line 161 and dies with IndexError. Raising a dataset-specific error here will make missing data or naming drift much easier to diagnose.
Suggested change
# Sort by time step
alterra_series.sort(key=lambda x: x[0])
+
+if not alterra_series:
+ raise FileNotFoundError(
+ f"No Alterra VTK files matching '*.t<number>.vtk' were found in {alterra_dir}"
+ )🤖 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_alterra_valve_to_usd.ipynb`
around lines 134 - 142, The code collects matches into alterra_series from
vtk_files using pattern but doesn't handle the case where no frames match;
before any access (e.g., before using alterra_series[0] later), check if
alterra_series is empty and raise a clear, dataset-specific exception (e.g.,
ValueError or a custom AlterraDatasetError) with a message that includes the
pattern and the source directory or vtk_files summary so callers can diagnose
missing frames/naming drift; perform this check immediately after
alterra_series.sort(...) and before downstream uses.
| "metadata": {}, | ||
| "source": [ | ||
| "## 3. Convert Full Time Series - TPV25" | ||
| "## 3. Convert TPV25" |
There was a problem hiding this comment.
Fix the section title.
This section is converting Alterra, not TPV25.
🤖 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_alterra_valve_to_usd.ipynb` at
line 202, The section header "## 3. Convert TPV25" is incorrect; update the
notebook markdown cell that contains that exact string (the header "## 3.
Convert TPV25") to accurately describe the conversion being performed, e.g.
change it to "## 3. Convert Alterra" or "## 3. Convert Alterra valve to USD" so
the section title matches the Alterra conversion code and context.
| "if conversion_settings.separate_objects_by_connectivity is True:\n", | ||
| " vessel_path = \"/World/Meshes/AlterraValve_object3\"\n", | ||
| "elif conversion_settings.separate_objects_by_cell_type is True:\n", | ||
| " vessel_path = \"/World/Meshes/AlterraValve_triangle1\"\n", | ||
| "else:\n", | ||
| " vessel_path = \"/World/Meshes/AlterraValve\"\n", |
There was a problem hiding this comment.
Discover the valve mesh path instead of pinning _object3.
With separate_objects_by_connectivity=True, the _objectN suffix depends on connected-component ordering during conversion, not on a stable valve identity. If that ordering changes, the primvar lookup at Line 268 will target the wrong mesh and the colormap step will silently stop applying to the valve.
| "colormap_primvar_substrs = [\"stress\", \"strain\"]\n", | ||
| "colormap_name = \"jet\" # matplotlib colormap name\n", | ||
| "colormap_range_min = 25\n", | ||
| "colormap_range_max = 200\n", |
There was a problem hiding this comment.
Restrict the colormap source to stress-only.
Using ["stress", "strain"] means this notebook can colorize TPV25 with a different scalar than Alterra while still reusing the same (25, 200) limits. That breaks the “consistent stress colormap” goal of the PR.
Suggested change
-colormap_primvar_substrs = ["stress", "strain"]
+colormap_primvar_substrs = ["stress"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "colormap_primvar_substrs = [\"stress\", \"strain\"]\n", | |
| "colormap_name = \"jet\" # matplotlib colormap name\n", | |
| "colormap_range_min = 25\n", | |
| "colormap_range_max = 200\n", | |
| colormap_primvar_substrs = ["stress"] | |
| colormap_name = "jet" # matplotlib colormap name | |
| colormap_range_min = 25 | |
| colormap_range_max = 200 |
🤖 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 94 - 97, The colormap source list colormap_primvar_substrs currently
allows both "stress" and "strain", which can let TPV25 use a different scalar
while still using the same colormap bounds
(colormap_range_min/colormap_range_max) and thus break the "consistent stress
colormap" requirement; update colormap_primvar_substrs to include only "stress"
so the notebook always uses stress as the scalar for colorization (leave
colormap_name and colormap_range_min/colormap_range_max as-is unless there’s a
separate range for strain to adjust).
| "tpv25_series = []\n", | ||
| "for vtk_file in vtk_files:\n", | ||
| " match = re.search(pattern, vtk_file.name)\n", | ||
| " if match:\n", | ||
| " time_step = int(match.group(1))\n", | ||
| " tpv25_series.append((time_step, vtk_file))\n", | ||
| "\n", | ||
| " # List mesh prims under /World/Meshes (each cell type is a separate prim)\n", | ||
| " meshes_prim = stage_ct.GetPrimAtPath(\"/World/Meshes\")\n", | ||
| " if meshes_prim:\n", | ||
| " children = meshes_prim.GetChildren()\n", | ||
| " print(f\"\\nMesh prims created (by cell type): {len(children)}\")\n", | ||
| " for child in children:\n", | ||
| " print(f\" - {child.GetPath().pathString}\")\n", | ||
| " print(f\"\\n✓ Saved: {output_by_cell_type}\")" | ||
| "# Sort by time step\n", | ||
| "tpv25_series.sort(key=lambda x: x[0])" |
There was a problem hiding this comment.
Fail fast when no TPV25 frames are found.
If no files survive the .t<number>.vtk filter, the notebook reaches tpv25_series[0] on Line 159 and fails with IndexError. A clear FileNotFoundError here will make broken paths or renamed files obvious.
Suggested change
# Sort by time step
tpv25_series.sort(key=lambda x: x[0])
+
+if not tpv25_series:
+ raise FileNotFoundError(
+ f"No TPV25 VTK files matching '*.t<number>.vtk' were found in {tpv25_dir}"
+ )🤖 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 132 - 140, The code collects matching VTK files into tpv25_series by
iterating over vtk_files with the regex pattern and then sorts it; add a guard
after tpv25_series.sort(...) that checks if tpv25_series is empty and raise a
FileNotFoundError with a clear message (including pattern and/or input directory
context) so later access like tpv25_series[0] fails fast and provides actionable
information. Ensure the check references the same variable name tpv25_series and
include enough context in the exception message to identify the missing TPV25
frames.
| "if conversion_settings.separate_objects_by_connectivity is True:\n", | ||
| " vessel_path = \"/World/Meshes/TPV25Valve_object4\"\n", | ||
| "elif conversion_settings.separate_objects_by_cell_type is True:\n", | ||
| " vessel_path = \"/World/Meshes/TPV25Valve_triangle1\"\n", | ||
| "else:\n", | ||
| " vessel_path = \"/World/Meshes/TPV25Valve\"\n", |
There was a problem hiding this comment.
Discover the valve mesh path instead of pinning _object4.
TPV25Valve_object4 is only stable as long as connected-component ordering never changes. Any shift there will make the primvar query at Line 266 inspect the wrong mesh, so the colormap can end up on the wrong component or not get applied at all.
🤖 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 258 - 263, The code pins vessel_path to a specific prim name
("TPV25Valve_object4"), which is brittle; instead detect the correct mesh prim
under the TPV25Valve meshes at runtime. Modify the logic around
conversion_settings.separate_objects_by_connectivity /
separate_objects_by_cell_type to enumerate children of
"/World/Meshes/TPV25Valve" (or the parent used), inspect prim names/attributes
or topology/point-count to find the intended mesh (e.g., the prim whose
topology/face-count or connectivity attribute matches the source VTK component),
and set vessel_path to that discovered prim path before the primvar lookup that
follows (the variable currently named vessel_path and the primvar query that
relies on it). Ensure fallback to the single mesh path when no separate objects
exist.
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
New Features
Chores