bugfixes#259
Merged
Merged
Conversation
Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
… variable dropdowns Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the GUI's plotting output functionality by splitting the single "Plot Output" tab into two separate tabs: "Plot Output 2D" and "Plot Output 1D". The changes introduce comprehensive support for 1D transect visualization and enhance the 2D plotting capabilities.
Key Changes:
- Split the output plotting interface into separate 2D and 1D visualization tabs
- Added variable selection dropdowns to both tabs for dynamic data exploration
- Replaced button-based plotting with automatic plot updates when files are loaded
- Introduced vegetation overlay checkbox for cleaner UI interaction
- Refactored path handling to use
os.pardirandos.sepfor cross-platform compatibility
Comments suppressed due to low confidence (1)
aeolis/gui.py:41
- The initialization sets
overlay_veg_enabledto False, but this attribute is no longer used in the refactored code. The new implementation usesself.overlay_veg_var(a BooleanVar) instead. This unused attribute should be removed.
self.nc_data_cache = None
self.overlay_veg_enabled = False
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def on_variable_changed(self, event): | ||
| """Update plot when variable selection changes""" | ||
| if hasattr(self, 'nc_data_cache_1d') and self.nc_data_cache_1d is not None: |
There was a problem hiding this comment.
The attribute nc_data_cache_1d is not initialized in __init__. Consider adding self.nc_data_cache_1d = None to the initialization to make the code's structure clearer and avoid potential AttributeError issues if code paths change.
| # Update plot if data is loaded | ||
| if hasattr(self, 'nc_data_cache_1d') and self.nc_data_cache_1d is not None: | ||
| # Reconfigure slider range based on new direction | ||
| first_var = list(self.nc_data_cache_1d['vars'].values())[0] |
There was a problem hiding this comment.
The code converts the entire dict_values object to a list just to get the first element. Use next(iter(self.nc_data_cache_1d['vars'].values())) instead for better performance, especially with large variable dictionaries.
Suggested change
| first_var = list(self.nc_data_cache_1d['vars'].values())[0] | |
| first_var = next(iter(self.nc_data_cache_1d['vars'].values())) |
Comment on lines
+741
to
+742
| coord_vars = {'x', 'y', 's', 'n', 'lat', 'lon', 'time', 'layers', 'fractions', | ||
| 'x_bounds', 'y_bounds', 'lat_bounds', 'lon_bounds', 'time_bounds', 'crs', 'nv', 'nv2'} |
There was a problem hiding this comment.
The coordinate variables set is duplicated in both plot_1d_transect (lines 741-742) and plot_nc_2d (lines 1004-1005). Consider defining this as a class constant or module-level constant to avoid duplication and maintain consistency.
Comment on lines
+810
to
+811
| # Use dict.values() directly instead of next(iter()) for clarity | ||
| first_var = list(var_data_dict.values())[0] |
There was a problem hiding this comment.
The code converts the entire dict_values object to a list just to get the first element. Use next(iter(var_data_dict.values())) instead for better performance.
Suggested change
| # Use dict.values() directly instead of next(iter()) for clarity | |
| first_var = list(var_data_dict.values())[0] | |
| # Use next(iter(var_data_dict.values())) for better performance | |
| first_var = next(iter(var_data_dict.values())) |
Comment on lines
+916
to
+932
| ylabel_dict = { | ||
| 'zb': 'Bed Elevation (m)', | ||
| 'ustar': 'Shear Velocity (m/s)', | ||
| 'ustars': 'Shear Velocity S-component (m/s)', | ||
| 'ustarn': 'Shear Velocity N-component (m/s)', | ||
| 'zs': 'Surface Elevation (m)', | ||
| 'zsep': 'Separation Elevation (m)', | ||
| 'Ct': 'Sediment Concentration (kg/m²)', | ||
| 'Cu': 'Equilibrium Concentration (kg/m²)', | ||
| 'q': 'Sediment Flux (kg/m/s)', | ||
| 'qs': 'Sediment Flux S-component (kg/m/s)', | ||
| 'qn': 'Sediment Flux N-component (kg/m/s)', | ||
| 'pickup': 'Sediment Entrainment (kg/m²)', | ||
| 'uth': 'Threshold Shear Velocity (m/s)', | ||
| 'w': 'Fraction Weight (-)', | ||
| } | ||
| ylabel = ylabel_dict.get(var_name, var_name) |
There was a problem hiding this comment.
The variable label/title dictionaries are duplicated across update_1d_plot, get_variable_label, and get_variable_title. Consider creating a single shared data structure (e.g., a class constant) that maps variables to both labels and titles to avoid inconsistencies and reduce duplication.
Suggested change
| ylabel_dict = { | |
| 'zb': 'Bed Elevation (m)', | |
| 'ustar': 'Shear Velocity (m/s)', | |
| 'ustars': 'Shear Velocity S-component (m/s)', | |
| 'ustarn': 'Shear Velocity N-component (m/s)', | |
| 'zs': 'Surface Elevation (m)', | |
| 'zsep': 'Separation Elevation (m)', | |
| 'Ct': 'Sediment Concentration (kg/m²)', | |
| 'Cu': 'Equilibrium Concentration (kg/m²)', | |
| 'q': 'Sediment Flux (kg/m/s)', | |
| 'qs': 'Sediment Flux S-component (kg/m/s)', | |
| 'qn': 'Sediment Flux N-component (kg/m/s)', | |
| 'pickup': 'Sediment Entrainment (kg/m²)', | |
| 'uth': 'Threshold Shear Velocity (m/s)', | |
| 'w': 'Fraction Weight (-)', | |
| } | |
| ylabel = ylabel_dict.get(var_name, var_name) | |
| ylabel = self.VARIABLE_LABELS.get(var_name, var_name) |
Comment on lines
+1004
to
+1005
| coord_vars = {'x', 'y', 's', 'n', 'lat', 'lon', 'time', 'layers', 'fractions', | ||
| 'x_bounds', 'y_bounds', 'lat_bounds', 'lon_bounds', 'time_bounds', 'crs', 'nv', 'nv2'} |
There was a problem hiding this comment.
The coordinate variables set is duplicated from plot_1d_transect. This duplication violates the DRY principle. Consider defining this as a class constant or module-level constant.
|
|
||
| # Browse button for NC file | ||
| nc_browse_btn_1d = ttk.Button(file_frame_1d, text="Browse...", | ||
| command=lambda: self.browse_nc_file_1d()) |
There was a problem hiding this comment.
This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
Suggested change
| command=lambda: self.browse_nc_file_1d()) | |
| command=self.browse_nc_file_1d) |
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
aeolis/gui.py:1046
- The comment states 'Need at least 3 dimensions: (time, n, s)' but does not mention the fractions dimension. This is inconsistent with the updated 1D plotting logic (line 778) which correctly documents '(time, n, s) or (time, n, s, fractions)'. The comment should be updated to include the fractions case for consistency.
# Need at least 3 dimensions: (time, n, s)
if var_data.ndim < 3:
continue # Skip variables without spatial dimensions
aeolis/gui.py:1052
- The 2D plotting function (plot_nc_2d) does not handle 3D variables without time dimension (n, s, fractions), unlike the 1D plotting function (plot_1d_transect) which correctly handles this case at lines 787-792. This inconsistency means 3D variables with fractions but no time dimension will be rejected in 2D plotting but accepted in 1D plotting. The logic should be updated to match the 1D implementation: check if var.ndim < 2 instead of != 2, and handle both 2D and 3D cases.
# Need exactly 2 spatial dimensions: (n, s)
if var.ndim != 2:
continue # Skip variables without 2D spatial dimensions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (4)
aeolis/gui.py:1425
- Except block directly handles BaseException.
except:
aeolis/gui.py:1558
- Except block directly handles BaseException.
except:
aeolis/gui.py:1562
- Except block directly handles BaseException.
except:
aeolis/gui.py:1645
- Except block directly handles BaseException.
except:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lz = math.sin(alt) | ||
|
|
||
| illum = np.clip(nx * lx + ny * ly + nz * lz, 0.0, 1.0) | ||
| shaded = 0.35 + (1.0 - 0.35) * illum # ambient term |
There was a problem hiding this comment.
The magic number 0.35 appears twice in the calculation. Consider extracting it as a named constant (e.g., AMBIENT_TERM = 0.35) at the module level or as a function parameter to improve code clarity and maintainability.
Comment on lines
+139
to
+143
| try: | ||
| self.plot_data('bed_file', 'Bed Elevation') | ||
| except Exception as e: | ||
| # Silently fail if plotting doesn't work (e.g., files don't exist) | ||
| pass |
There was a problem hiding this comment.
The exception is caught but the variable 'e' is unused. Either remove the variable (use bare except Exception:) or log the error for debugging purposes instead of silently failing. Silent failures can make troubleshooting difficult.
| self.output_colorbar.update_normal(im) | ||
| cbar_label = self.get_variable_label(var_name) | ||
| self.output_colorbar.set_label(cbar_label) | ||
| except: |
There was a problem hiding this comment.
Bare except: clause catches all exceptions including system exits and keyboard interrupts. Use except Exception: to catch only regular exceptions, or specify the expected exception types (e.g., except (AttributeError, ValueError):).
Suggested change
| except: | |
| except Exception: |
Comment on lines
+1558
to
+1562
| except: | ||
| # If remove() fails, try removing from figure | ||
| try: | ||
| self.output_fig.delaxes(self.output_colorbar.ax) | ||
| except: |
There was a problem hiding this comment.
Multiple bare except: clauses. Use except Exception: or specific exception types to avoid catching system exits and keyboard interrupts. This appears in nested try-except blocks.
Suggested change
| except: | |
| # If remove() fails, try removing from figure | |
| try: | |
| self.output_fig.delaxes(self.output_colorbar.ax) | |
| except: | |
| except Exception: | |
| # If remove() fails, try removing from figure | |
| try: | |
| self.output_fig.delaxes(self.output_colorbar.ax) | |
| except Exception: |
| try: | ||
| self.output_colorbar.update_normal(im) | ||
| self.output_colorbar.set_label('Shear Velocity (m/s)') | ||
| except: |
There was a problem hiding this comment.
Bare except: clause. Use except Exception: or specify the expected exception types to avoid catching system exits and keyboard interrupts.
Suggested change
| except: | |
| except Exception: |
Comment on lines
+1685
to
+1687
| q = self.output_ax.quiver(X_plot, Y_plot, U_plot, V_plot, | ||
| color='black', scale=None, scale_units='xy', | ||
| angles='xy', pivot='mid', width=0.003) |
There was a problem hiding this comment.
The magic number 0.003 is used for quiver arrow width. Consider extracting this as a named constant (e.g., QUIVER_ARROW_WIDTH = 0.003) to improve maintainability.
| # Calculate reference vector magnitude for quiver key | ||
| magnitude_all = np.sqrt(U_plot**2 + V_plot**2) | ||
| if magnitude_all.max() > 0: | ||
| ref_magnitude = magnitude_all.max() * 0.5 |
There was a problem hiding this comment.
The magic number 0.5 is used to calculate reference magnitude for the quiver key. Consider extracting this as a named constant (e.g., QUIVER_KEY_SCALE_FACTOR = 0.5) to improve code clarity.
| # Apply ocean mask: zb < -0.5 and x < 200 | ||
| if x_data is not None: | ||
| X2d, _ = np.meshgrid(x1d, y1d) | ||
| ocean_mask = (zb < -0.5) & (X2d < 200) |
There was a problem hiding this comment.
The magic numbers -0.5 and 200 are used for ocean masking criteria. Consider extracting these as named constants (e.g., OCEAN_DEPTH_THRESHOLD = -0.5, OCEAN_X_LIMIT = 200) to improve code clarity and maintainability.
| magnitude_all = np.sqrt(U_plot**2 + V_plot**2) | ||
| if magnitude_all.max() > 0: | ||
| ref_magnitude = magnitude_all.max() * 0.5 | ||
| qk = self.output_ax.quiverkey(q, 0.9, 0.95, ref_magnitude, |
There was a problem hiding this comment.
Variable qk is not used.
Suggested change
| qk = self.output_ax.quiverkey(q, 0.9, 0.95, ref_magnitude, | |
| self.output_ax.quiverkey(q, 0.9, 0.95, ref_magnitude, |
Comment on lines
+1562
to
+1563
| except: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except: | |
| pass | |
| except Exception as e: | |
| # Failed to remove colorbar axes; log and continue | |
| import traceback | |
| print(f"Failed to remove colorbar axes: {e}") | |
| traceback.print_exc() |
Sierd
added a commit
that referenced
this pull request
Nov 6, 2025
* Initialisation of the GUI Including some overdue maintenance related to python 2-->3 in the write_configfile script. * Updated GUI structure using Class * updated gui * update gui branche with latest main (#255) * fixed dependencies for python 3.13 (#236) * test with timing function * Add devcontainer for use in codespaces (#240) * devcontainer added * Update Dockerfile * Update Dockerfile * Update devcontainer.json * Update Dockerfile * fixed dependencies for python 3.13 (#236) (#239) * Update Dockerfile * remove abundant stuff * Add devcontainer for use in codespaces (#240) (#242) * devcontainer added * Update Dockerfile * Update Dockerfile * Update devcontainer.json * Update Dockerfile * fixed dependencies for python 3.13 (#236) (#239) * Update Dockerfile * remove abundant stuff * Update Dockerfile (#243) This patch updates the dockerfile for the codespace to be compatible with developer mode. You need to manual install python in dev model after the docker is created. * Cleanup main (#245) * delete abundant files * deleted: aeolis/examples/vanWesten2024/blowout/figure_grid_initialization.png deleted: aeolis/examples/vanWesten2024/blowout/figure_params_initialization.png deleted: aeolis/examples/vanWesten2024/blowout/figure_timeseries_initialization.png * Update pyproject.toml * Update README.md * Update CITATION.cff * Update release info (#246) * Update pyproject.toml * Update CITATION.cff * Update README.md * Update Python version and dependencies in installation guide * Implement error message for missing ne_file Added error handling for missing 'ne_file' when using Avalanching. * Print message for aeolis installation in Dockerfile Add message indicating manual installation of aeolis * Update vegetation parameters in constants.py (#250) * Update vegetation parameters in constants.py Removes unused V_Lat parameter solves #120 Changes default for veg_min_elevation to -10 to avoid unwanted use of this functionality. * Update aeolis/constants.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * input and output tab added improved domain plotting functionality * added functionality * Fix GUI crash when canceling file selection on startup (#256) * Initial plan * Fix GUI to handle cancel on startup gracefully Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Address code review feedback: move import to top and remove hardcoded path Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Remove placeholder path, set configfile to 'No file selected' when canceled Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Add Plot Output 1D tab for transect visualization with unified interface (#257) * Initial plan * Add Plot Output 1D tab with transect plotting functionality Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Add variable change callback for 1D plot updates Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Fix code review issues: cross-platform paths and dimension validation Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Improve dimension validation and dictionary access robustness Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Fix transect direction bug and unify 2D/1D tab interface with dynamic variable dropdowns Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * bugfixes (#259) * Initial plan * Add Plot Output 1D tab with transect plotting functionality Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Add variable change callback for 1D plot updates Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Fix code review issues: cross-platform paths and dimension validation Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Improve dimension validation and dictionary access robustness Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * Fix transect direction bug and unify 2D/1D tab interface with dynamic variable dropdowns Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * bugfixes * added functionality, colorbars, many small fixes. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com> * domain overview added to 1D plot mode. * Delete aeolis/GUI.ipynb * Remove incompatible parameter checks for ne_file Removed error handling for missing ne_file when using Avalanching. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
Merged
Sierd
added a commit
that referenced
this pull request
Nov 6, 2025
* Initialisation of the GUI Including some overdue maintenance related to python 2-->3 in the write_configfile script. * Updated GUI structure using Class * updated gui * update gui branche with latest main (#255) * fixed dependencies for python 3.13 (#236) * test with timing function * Add devcontainer for use in codespaces (#240) * devcontainer added * Update Dockerfile * Update Dockerfile * Update devcontainer.json * Update Dockerfile * fixed dependencies for python 3.13 (#236) (#239) * Update Dockerfile * remove abundant stuff * Add devcontainer for use in codespaces (#240) (#242) * devcontainer added * Update Dockerfile * Update Dockerfile * Update devcontainer.json * Update Dockerfile * fixed dependencies for python 3.13 (#236) (#239) * Update Dockerfile * remove abundant stuff * Update Dockerfile (#243) This patch updates the dockerfile for the codespace to be compatible with developer mode. You need to manual install python in dev model after the docker is created. * Cleanup main (#245) * delete abundant files * deleted: aeolis/examples/vanWesten2024/blowout/figure_grid_initialization.png deleted: aeolis/examples/vanWesten2024/blowout/figure_params_initialization.png deleted: aeolis/examples/vanWesten2024/blowout/figure_timeseries_initialization.png * Update pyproject.toml * Update README.md * Update CITATION.cff * Update release info (#246) * Update pyproject.toml * Update CITATION.cff * Update README.md * Update Python version and dependencies in installation guide * Implement error message for missing ne_file Added error handling for missing 'ne_file' when using Avalanching. * Print message for aeolis installation in Dockerfile Add message indicating manual installation of aeolis * Update vegetation parameters in constants.py (#250) * Update vegetation parameters in constants.py Removes unused V_Lat parameter solves #120 Changes default for veg_min_elevation to -10 to avoid unwanted use of this functionality. * Update aeolis/constants.py --------- --------- * input and output tab added improved domain plotting functionality * added functionality * Fix GUI crash when canceling file selection on startup (#256) * Initial plan * Fix GUI to handle cancel on startup gracefully * Address code review feedback: move import to top and remove hardcoded path * Remove placeholder path, set configfile to 'No file selected' when canceled --------- * Add Plot Output 1D tab for transect visualization with unified interface (#257) * Initial plan * Add Plot Output 1D tab with transect plotting functionality * Add variable change callback for 1D plot updates * Fix code review issues: cross-platform paths and dimension validation * Improve dimension validation and dictionary access robustness * Fix transect direction bug and unify 2D/1D tab interface with dynamic variable dropdowns --------- * bugfixes (#259) * Initial plan * Add Plot Output 1D tab with transect plotting functionality * Add variable change callback for 1D plot updates * Fix code review issues: cross-platform paths and dimension validation * Improve dimension validation and dictionary access robustness * Fix transect direction bug and unify 2D/1D tab interface with dynamic variable dropdowns * bugfixes * added functionality, colorbars, many small fixes. --------- * domain overview added to 1D plot mode. * Delete aeolis/GUI.ipynb * Remove incompatible parameter checks for ne_file Removed error handling for missing ne_file when using Avalanching. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Sierd <14054272+Sierd@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.