Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the equation of state conserved limiter for moist physics and refactors the Python binding API for mesh block initialization. The changes address an uninitialized variable issue that could affect restart functionality in ideal moist simulations.
Changes:
- Fixed uninitialized variable bug in
apply_conserved_limiter_by computing vapor/cloud counts at the beginning of the function - Reordered energy limiter to run before vapor/cloud processing to ensure proper energy bounds on restart
- Split Python binding
initializemethod into two separate methods:initialize(for normal initialization) andinitialize_from_restart(for restart initialization)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/eos/equation_of_state.cpp | Fixes undefined behavior by initializing ny, nvapor, and ncloud before use; reorders energy limiter to run before vapor processing |
| python/csrc/pymesh.cpp | Splits initialization method into two separate methods for clarity, but introduces a breaking API change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "initialize", | ||
| [](snap::MeshBlockImpl &self, snap::Variables &vars) { | ||
| self.initialize(vars); | ||
| return std::make_pair(vars, 0.); | ||
| }, | ||
| py::arg("vars")) | ||
| .def( | ||
| "initialize_from_restart", | ||
| [](snap::MeshBlockImpl &self, snap::Variables &vars, | ||
| std::string restart_file) { | ||
| double time; | ||
| if (restart_file.empty()) { | ||
| time = self.initialize(vars); | ||
| } else { | ||
| time = self.initialize(vars, restart_file.c_str()); | ||
| } | ||
| double time = self.initialize(vars, restart_file.c_str()); | ||
| return std::make_pair(vars, time); | ||
| }, | ||
| py::arg("vars"), py::arg("restart_file") = "") | ||
| py::arg("vars"), py::arg("restart_file")) |
There was a problem hiding this comment.
Breaking API change: Splitting the initialize method into two separate methods breaks backward compatibility. The original API supported initialize(vars, restart_file="") where an empty string would perform normal initialization and a non-empty string would initialize from a restart file.
The new API requires callers to choose between initialize(vars) or initialize_from_restart(vars, restart_file). While this is clearer and more explicit, any existing code that passes a restart file to initialize will break.
Consider one of the following approaches:
- Deprecate the old API first before removing it, providing a migration path
- Keep backward compatibility by supporting both the old combined method and the new separate methods
- Document this breaking change clearly in release notes and provide migration guidance
Note: While no Python examples in this repository currently use restart files with the initialize method, external code may rely on this API.
|
🎉 Released v1.2.9! What's Changed
Full Changelog: v1.2.8...v1.2.9 |
1 participant
No description provided.