Conversation
There was a problem hiding this comment.
Pull request overview
Updates ensemble/plot defaults and improves visualization consistency for the ISMIP_Choi example, focusing on clearer figures and more reproducible parameter choices.
Changes:
- Adjusted default ensemble-generation parameters (Ne, radar stride, measurement noise).
- Standardized difference-panel color scaling via shared symmetric limits and shared colorbars.
- Refined figure sizing/layout and updated velocity unit labeling in plots.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| applications/issm_model/examples/ISMIP_Choi/read_results.m | Improves plot layouts and enforces shared/symmetric color limits for difference panels; updates some labels/toggles. |
| applications/issm_model/examples/ISMIP_Choi/generate_bed_kringing.py | Updates default CLI and function parameters for ensemble generation settings. |
Comments suppressed due to low confidence (2)
applications/issm_model/examples/ISMIP_Choi/read_results.m:1
diff_nois used to computemaxAbs_nobut is not assigned anywhere in the shown logic (all candidate assignments are commented out). This will error at runtime. Re-introduce an explicitdiff_no = ...assignment (difference vs relative error vs log-relative error) beforemax(abs(diff_no(:))), and ensure the colorbar label matches that choice.
%% -----------------------------------------------------------
applications/issm_model/examples/ISMIP_Choi/generate_bed_kringing.py:45
stride_km: float = 4uses an integer literal for a float-typed parameter. While Python will accept it, using4.0improves clarity/consistency with the annotation and avoids confusion when reading defaults.
def generate_bed_kriging(
icesee_path: str = "./",
data_path: str = "_modelrun_datasets",
Ne: int = 100,
stride_km: float = 4,
snap_idx: int = 0,
sigma_noise: float = 5.0,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i = 2:nrows | ||
| colormap(axs(i), redblue(256)); | ||
| caxis(axs(i), [-maxAbs_global maxAbs_global]); | ||
| end | ||
| cb2 = colorbar(axs(end), 'Position',[colorbar_gap 0.25 0.015 0.40]); | ||
| % ylabel(cb2,'Difference','FontSize',12,'FontWeight','bold'); | ||
| % if contains(field,'geometry.bed') | ||
| ylabel(cb2,'Relative Error','FontSize',15,'FontWeight','bold'); | ||
| ylabel(cb2,'Relative Error','FontSize',15,'FontWeight','bold'); | ||
| % if contains(field, 'initialization.vel') | ||
| % ylabel(cb2,['\Delta |u|' units_str],'FontSize',15,'FontWeight','bold'); | ||
| % else | ||
| % ylabel(cb2,['Difference' units_str],'FontSize',12,'FontWeight','bold'); | ||
| % ylabel(cb2,['\Delta ' field_title units_str],'FontSize',15,'FontWeight','bold'); |
There was a problem hiding this comment.
The diff colorbar is always labeled “Relative Error”, but the function (and surrounding commented code) indicates the plotted quantity may be a raw difference (ens - true) in many cases. This mislabels figures and can lead to incorrect interpretation. Either (a) set the label based on the actual computation used for diff_no/diff_k, or (b) ensure diff_no/diff_k are always computed as relative error if the label remains “Relative Error”.
| parser.add_argument( | ||
| "--Ne", | ||
| type=int, | ||
| default=60, | ||
| default=100, | ||
| help="Number of ensemble realizations to generate (default: 60)", | ||
| ) | ||
| parser.add_argument( | ||
| "--stride-km", | ||
| type=float, | ||
| default=7.0, | ||
| default=4, | ||
| help="Radar track spacing in km (default: 7.0)", | ||
| ) |
There was a problem hiding this comment.
The CLI help strings list old default values (60 / 7.0 / 10.0) that no longer match the actual defaults (100 / 4 / 5.0). Update the help text to reflect the new defaults so --help output is correct. (Minor: consider using default=4.0 for --stride-km to match the float type and help formatting.)
| parser.add_argument( | ||
| "--sigma-noise", | ||
| type=float, | ||
| default=10.0, | ||
| default=5.0, | ||
| help="Standard deviation of radar measurement noise in meters (default: 10.0)", | ||
| ) |
There was a problem hiding this comment.
The CLI help strings list old default values (60 / 7.0 / 10.0) that no longer match the actual defaults (100 / 4 / 5.0). Update the help text to reflect the new defaults so --help output is correct. (Minor: consider using default=4.0 for --stride-km to match the float type and help formatting.)
This pull request makes several parameter adjustments and visualization improvements to the ensemble generation and results plotting scripts for the ISMIP_Choi example. The main changes include updating default values for ensemble generation, improving the consistency and clarity of plots, and refining color axis handling for difference panels. These updates enhance reproducibility and readability of experiment results.
Parameter updates for ensemble generation (
generate_bed_kriging.py):Ne) from 60 to 100 and reduced the radar track spacing (stride_km) from 10.0/7.0 to 4 km for finer spatial resolution. Also reduced the default radar measurement noise (sigma_noise) from 15.0/10.0 to 5.0 for more realistic uncertainty modeling. [1] [2] [3]Plotting and visualization improvements (
read_results.m):m/stom/yrfor clarity and domain consistency.These changes collectively improve the experiment setup and the quality of output figures, making the results easier to interpret and compare across different runs.