Skip to content

Add extrap options#67

Merged
chengcli merged 2 commits intomainfrom
cli/extrap_opts
Dec 16, 2025
Merged

Add extrap options#67
chengcli merged 2 commits intomainfrom
cli/extrap_opts

Conversation

@chengcli
Copy link
Owner

No description provided.

Copilot AI review requested due to automatic review settings December 16, 2025 14:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the adiabatic extrapolation API by replacing two overloaded extrapolate_ad functions with more descriptive names (extrapolate_dlnp and extrapolate_dz) and introducing an ExtrapOptions struct to encapsulate the parameters. This improves code clarity by making the function names more explicit about their purpose and consolidating related parameters into a structured options object.

Key Changes:

  • Introduced ExtrapOptions struct to encapsulate extrapolation parameters (dlnp, dz, grav, ds_dlnp, ds_dz, verbose)
  • Renamed extrapolate_ad(temp, pres, xfrac, dlnp, ...) to extrapolate_dlnp(temp, pres, xfrac, ExtrapOptions)
  • Renamed extrapolate_ad(temp, pres, xfrac, grav, dz, ...) to extrapolate_dz(temp, pres, xfrac, ExtrapOptions)

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/thermo/thermo.hpp Added ExtrapOptions struct definition and updated function signatures to use the new options-based API
src/thermo/extrapolate_ad.cpp Renamed functions to extrapolate_dlnp and extrapolate_dz, updated implementations to extract parameters from ExtrapOptions
python/csrc/pythermo.cpp Updated Python bindings with lambda wrappers that convert individual parameters to ExtrapOptions for backward compatibility
tests/test_thermo.cpp Updated test to use new extrapolate_dlnp function with ExtrapOptions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ADD_ARG(double, dlnp) = 0.;
ADD_ARG(double, dz) = 0.;
// Gravitational acceleration (m/s^2), positive
ADD_ARG(double, grav) = 0;
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value for grav should be 0. (with decimal point) to be consistent with other double-type fields in the struct (like dlnp, dz, ds_dlnp, and ds_dz which all use 0.). Currently it's 0 without the decimal point.

Suggested change
ADD_ARG(double, grav) = 0;
ADD_ARG(double, grav) = 0.;

Copilot uses AI. Check for mistakes.
Comment on lines +86 to +90
ADD_ARG(double, dz) = 0.;
// Gravitational acceleration (m/s^2), positive
ADD_ARG(double, grav) = 0;
ADD_ARG(double, ds_dlnp) = 0;
// Height change (m)
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments for struct fields are misplaced. The comment "// Height change (m)" on line 90 should appear before the dz field on line 86, not before ds_dz. Similarly, the comment "// Gravitational acceleration (m/s^2), positive" on line 87-88 should appear before the grav field on line 88. Consider reorganizing the fields and comments so that each comment directly precedes its corresponding field definition.

Suggested change
ADD_ARG(double, dz) = 0.;
// Gravitational acceleration (m/s^2), positive
ADD_ARG(double, grav) = 0;
ADD_ARG(double, ds_dlnp) = 0;
// Height change (m)
// Height change (m)
ADD_ARG(double, dz) = 0.;
// Gravitational acceleration (m/s^2), positive
ADD_ARG(double, grav) = 0;
ADD_ARG(double, ds_dlnp) = 0;

Copilot uses AI. Check for mistakes.
Comment on lines +89 to +96
torch::Tensor xfrac, double dz, double grav, double ds_dz,
bool verbose) {
kintera::ExtrapOptions opts;
opts.dz(dz).grav(grav).ds_dz(ds_dz).verbose(verbose);
return self.extrapolate_dz(temp, pres, xfrac, opts);
},
py::arg("temp"), py::arg("pres"), py::arg("xfrac"), py::arg("dz"),
py::arg("grav"), py::arg("ds_dz") = 0., py::arg("verbose") = false);
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter order for extrapolate_dz has been changed from the original (temp, pres, xfrac, grav, dz, ds_dz, verbose) to (temp, pres, xfrac, dz, grav, ds_dz, verbose). This is a breaking API change - the positions of dz and grav have been swapped. Existing Python code using this function will silently get incorrect results (e.g., passing grav=9.8, dz=100 will be interpreted as dz=9.8, grav=100). Either restore the original parameter order or ensure this breaking change is intentional and well-documented.

Suggested change
torch::Tensor xfrac, double dz, double grav, double ds_dz,
bool verbose) {
kintera::ExtrapOptions opts;
opts.dz(dz).grav(grav).ds_dz(ds_dz).verbose(verbose);
return self.extrapolate_dz(temp, pres, xfrac, opts);
},
py::arg("temp"), py::arg("pres"), py::arg("xfrac"), py::arg("dz"),
py::arg("grav"), py::arg("ds_dz") = 0., py::arg("verbose") = false);
torch::Tensor xfrac, double grav, double dz, double ds_dz,
bool verbose) {
kintera::ExtrapOptions opts;
opts.grav(grav).dz(dz).ds_dz(ds_dz).verbose(verbose);
return self.extrapolate_dz(temp, pres, xfrac, opts);
},
py::arg("temp"), py::arg("pres"), py::arg("xfrac"), py::arg("grav"),
py::arg("dz"), py::arg("ds_dz") = 0., py::arg("verbose") = false);

Copilot uses AI. Check for mistakes.
@chengcli chengcli merged commit 73b3924 into main Dec 16, 2025
3 checks passed
@chengcli chengcli deleted the cli/extrap_opts branch December 16, 2025 15:09
@github-actions
Copy link

🎉 Released v1.2.7!

What's Changed

Full Changelog: v1.2.6...v1.2.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant