Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds clone() methods to several options implementation structs throughout the codebase to enable deep copying of configuration objects. The PR also updates the pydisort dependency from version 1.4.1 to 1.4.5, which is likely required to support the DisortOptions::clone() method used in the new implementations. Additionally, the PR improves the formatting of the report() method in RadiationBandOptionsImpl.
Changes:
- Added
clone()methods to five options implementation structs:ToonMcKay89OptionsImpl,RadiationBandOptionsImpl,RadiationOptionsImpl,OpacityOptionsImpl, andIntegratorOptionsImpl - Bumped pydisort version requirement from 1.4.1 to 1.4.5 across build configurations
- Enhanced report formatting in
RadiationBandOptionsImplto usefmt::formatand conditionally display solver options
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/rtsolver/toon_mckay89.hpp | Added clone method for ToonMcKay89OptionsImpl using copy constructor |
| src/radiation/radiation_band.hpp | Added clone method with deep copying for disort and toon options; improved report formatting with fmt; added harp_formatter.hpp include |
| src/radiation/radiation.hpp | Added clone method that deep clones the bands vector |
| src/opacity/opacity_options.hpp | Added clone method for OpacityOptionsImpl using copy constructor |
| src/integrator/integrator.hpp | Added clone method for IntegratorOptionsImpl using copy constructor |
| pyproject.toml | Updated pydisort version requirement from 1.4.1 to 1.4.5 in both build-system.requires and dependencies |
| .github/workflows/ci.yml | Updated pydisort version in CI workflow pip install command |
| .github/workflows/cd.yml | Updated pydisort version in CD workflow for both macOS and Linux builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::shared_ptr<RadiationBandOptionsImpl> clone() const { | ||
| auto op = std::make_shared<RadiationBandOptionsImpl>(*this); | ||
| if (op->disort() != nullptr) { | ||
| op->disort() = op->disort()->clone(); | ||
| } | ||
| if (op->toon() != nullptr) { | ||
| op->toon() = op->toon()->clone(); | ||
| } | ||
| return op; | ||
| } |
There was a problem hiding this comment.
The clone method performs a shallow copy of the opacities map. Since OpacityDict is a map of shared_ptr (OpacityOptions), the cloned RadiationBandOptionsImpl will share the same OpacityOptionsImpl objects with the original. This means modifying opacity options in the clone will affect the original and vice versa.
Consider deep cloning the opacities map as well to ensure true independence between the clone and the original:
if (op->opacities().size() > 0) {
auto& opacities_ref = op->opacities();
opacities_ref.clear();
for (auto const& [k, v] : opacities()) {
opacities_ref[k] = v->clone();
}
}There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
…82) * Initial plan * Add deep cloning for opacities map in clone method Co-authored-by: chengcli <69489965+chengcli@users.noreply.github.com> * Final update: deep cloning implementation complete Co-authored-by: chengcli <69489965+chengcli@users.noreply.github.com> * Remove CodeQL artifact file Co-authored-by: chengcli <69489965+chengcli@users.noreply.github.com> * Update src/radiation/radiation_band.hpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: chengcli <69489965+chengcli@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
🎉 Released v1.8.8! What's Changed
Full Changelog: v1.8.7...v1.8.8 |
No description provided.