Changed behaviour with setPT()#16
Merged
sdmytrievs merged 19 commits intogemshub:masterfrom Apr 27, 2026
Merged
Conversation
Merge pull request gemshub#14 from sdmytrievs/master
gdmiron
previously approved these changes
Mar 6, 2026
… the name phase with the species.
…er, the name phase with the species.
Check limits
Member
|
There are functions using index like: we should also add here, and similar functions, an optional phase index Check that we have overloaded functions that use index / name to get properties. if we have function(index) we should have overloaded function(name) as well |
This was
linked to
issues
Mar 20, 2026
Closed
…s, and added functions without bounds checking.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds name-based and optional-phase overloads plus unchecked Changes
sequenceDiagram
autonumber
participant Demo as Demo (C++ / Python)
participant Bind as PyBind Layer
participant Engine as xGEMS::ChemicalEngine
participant Data as Thermo Resources
rect rgba(200,230,255,0.5)
Demo->>Bind: call equilibrate / setPT / indexSpecies / species ops
end
rect rgba(200,255,200,0.5)
Bind->>Engine: invoke bound method (name or index/_i overload)
Engine->>Engine: resolve name→index (optional phase), validate (_i vs checked)
Engine-->>Bind: return value / bool / warning
end
rect rgba(255,230,200,0.5)
Engine->>Data: load/query dataset entries (if needed)
Data-->>Engine: return records
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/src/xgems/pyxGEMS.hpp (1)
791-806:⚠️ Potential issue | 🔴 CriticalSwap the Python
setPTparameter names.
ChemicalEngine::setPTis declared as(P, T), but this binding exposes(temperature, pressure). Positional documentation is inverted, and keyword calls will pass the values into the wrong native parameters.Suggested fix
- :param float temperature: Temperature in Kelvin (K). - :param float pressure: Pressure in Pascals (Pa). + :param float pressure: Pressure in Pascals (Pa). + :param float temperature: Temperature in Kelvin (K). @@ - engine.setPT(298.15, 101325) + engine.setPT(101325, 298.15) )doc", - py::arg("temperature"), py::arg("pressure")) + py::arg("pressure"), py::arg("temperature"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/src/xgems/pyxGEMS.hpp` around lines 791 - 806, The pybind11 binding for ChemicalEngine::setPT currently exposes arguments as py::arg("temperature"), py::arg("pressure") but the native signature is (P, T); swap the argument order and names to py::arg("pressure"), py::arg("temperature") so positional and keyword calls map correctly, and update the docstring parameter names and descriptions to list pressure first then temperature to match ChemicalEngine::setPT.xGEMS/ChemicalEngineMaps.cpp (1)
163-169:⚠️ Potential issue | 🟠 MajorRestrict aqueous molarity output to aqueous species.
This loop walks every species in the system and divides each amount by the aqueous phase volume, so solids and gases are reported as aqueous concentrations too. Iterate only over the aqueous phase slice, like
aq_species_molality()already does.Suggested fix
- auto moles_species = gem.speciesAmounts(); - for(Index i = 0; i < nspecies(); ++i) { - out[gem.speciesName_i(i)] = moles_species[i] / (gem.phaseVolume_i(aq_index)*1000); // volume from m3 to L + auto moles_species = gem.speciesAmounts(); + auto first = gem.indexFirstSpeciesInPhase_i(aq_index); + auto last = first + gem.numSpeciesInPhase_i(aq_index); + for(Index i = first; i < last; ++i) { + out[gem.speciesName_i(i)] = moles_species[i] / (gem.phaseVolume_i(aq_index)*1000); // volume from m3 to L }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xGEMS/ChemicalEngineMaps.cpp` around lines 163 - 169, The loop currently iterates all species (nspecies()) and divides each amount by the aqueous volume, producing aqueous concentrations for non-aqueous species; change it to iterate only the aqueous-phase species slice (use the existing aq_species_molality()/or equivalent accessor) instead of nspecies(), and compute molarity by dividing only those aqueous-species moles (from moles_species or the aq slice) by gem.phaseVolume_i(aq_index)*1000; update the keys using gem.speciesName_i(i) for the aqueous-species indices and leave non-aqueous species out of ValuesMap.
🧹 Nitpick comments (3)
xGEMS/ChemicalEngineMaps.hpp (1)
143-143: Use a valid Doxygen param name forphase.
@param optional phase ...is malformed; use@param phase ...and describe optionality in the text.📝 Suggested doc fix pattern
- * `@param` optional phase (std::string) Name of the phase the species was included in. If default get the first index. + * `@param` phase (std::optional<std::string>) Optional phase name used to disambiguate species lookup; when omitted, the first matching species is used.Also applies to: 742-743, 834-835, 849-850, 918-919, 977-977
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xGEMS/ChemicalEngineMaps.hpp` at line 143, The Doxygen `@param` tags incorrectly use "@param optional phase ..." — update each occurrence to "@param phase ..." and move any "optional" wording into the descriptive text (e.g., "phase (optional): ..." or "If omitted, defaults to the first index") so the param name is a valid Doxygen identifier; apply this fix for the instances around the current diff and the additional occurrences noted (near lines ~742-743, ~834-835, ~849-850, ~918-919, ~977) ensuring the parameter name 'phase' remains consistent in comments for the associated functions/classes in ChemicalEngineMaps.hpp.xGEMS/ChemicalEngine.cpp (2)
484-497: Minor: Comment refers to "element" but should say "species".Line 496 comment says "in case that element name was not found" but this function looks up species, not elements.
📝 Proposed fix
- return (ispecies>=0 ? ispecies : numSpecies()); // in case that element name was not found + return (ispecies>=0 ? ispecies : numSpecies()); // in case that species name was not found🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xGEMS/ChemicalEngine.cpp` around lines 484 - 497, The trailing comment in ChemicalEngine::indexSpecies incorrectly says "element" when it should say "species"; update the comment on the return statement (inside function ChemicalEngine::indexSpecies) to read something like "in case that species name was not found" so it accurately reflects that the function looks up species (and not elements).
920-927: Remove commented-out dead code.Line 926 has
//return Vector{};which appears to be leftover from a previous implementation.🧹 Proposed cleanup
auto ChemicalEngine::chemicalPotentials() const -> VectorConstRef { for (Index i = 0; i < numSpecies(); ++i) pimpl->chemPotentials[i] = pimpl->node->Get_muDC(i, false); return pimpl->chemPotentials; - - //return Vector{}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xGEMS/ChemicalEngine.cpp` around lines 920 - 927, In ChemicalEngine::chemicalPotentials(), remove the commented-out dead code "//return Vector{};" so the function body only assigns pimpl->chemPotentials via pimpl->node->Get_muDC(i, false) and returns pimpl->chemPotentials; locate the method by name (ChemicalEngine::chemicalPotentials) and the members pimpl->chemPotentials and pimpl->node->Get_muDC to safely delete that leftover comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/demo2.py`:
- Around line 33-34: The second print statement mislabels the arguments: it
prints "setPT(101325, 673.15)" while actually calling engine.setPT(50000000,
673.15); update the printed string to match the real call (e.g., change the
literal in the second print to "setPT(50000000, 673.15)") so the demo output
correctly reflects the arguments passed to engine.setPT.
In `@environment.devenv.yml`:
- Line 21: The conda spec "gems3k=>4.5.2" uses an invalid comparator; update the
package spec to use a supported comparator by replacing "gems3k=>4.5.2" with
"gems3k>=4.5.2" so the MatchSpec is valid.
In `@xGEMS/ChemicalEngine.cpp`:
- Around line 580-587: The string overload
ChemicalEngine::indexFirstSpeciesInPhase(std::string phase) returns 0 on an
invalid phase name, which is inconsistent with the Index overload
indexFirstSpeciesInPhase(Index) that returns numSpecies() for invalid indices;
change the function to call indexPhase(phase), check iphase<numPhases() and on
failure return numSpecies() instead of 0 so callers using result < numSpecies()
behave consistently; update the return in
ChemicalEngine::indexFirstSpeciesInPhase to return numSpecies() and keep the
valid branch using indexFirstSpeciesInPhase_i(iphase).
In `@xGEMS/ChemicalEngine.hpp`:
- Around line 396-409: The getters that take an optional phase currently declare
the parameter as mandatory; update their declarations to provide a default of
std::nullopt so one-argument calls work (e.g. change speciesCharge(std::string
species, std::optional<std::string> phase) const -> double to accept phase =
std::nullopt). Apply the same fix to the other getter overloads that mirror
indexSpecies/speciesAmount/setter patterns (for example standardMolarVolume and
the other getter signatures called out in the comment) so all phase parameters
are consistently defaulted to std::nullopt.
In `@xGEMS/ChemicalEngineMaps.cpp`:
- Around line 63-70: The species metadata maps (m_species_charges,
m_species_molar_mass, m_species_molar_volumes) are keyed only by specname and
get overwritten when the same species name appears in multiple phases (e.g.,
series1/Albite); change them to use a phase-qualified key so entries are unique
per phase. In the loop that iterates nspecies() using gem.speciesName_i(i),
build a composite key (e.g., phase + ":" + specname) using the phase identifier
available from the gem (such as gem.speciesPhase_i(i) or
gem.speciesPhaseName_i(i)) and store/look up values in m_species_charges,
m_species_molar_mass, and m_species_molar_volumes using that composite key; also
push the phase-qualified name into m_species_names (or maintain a separate map
from phase-qualified -> bare name) so downstream kg/m³ conversions and flat
dictionaries are unambiguous.
- Around line 148-154: The code uses a phase-local index
(numSpeciesInPhase_i(aq_index)-1) and divides by 1000, causing wrong species
selection and a 1000x error; replace the phase-local H2O lookup with the global
aqueous H2O index provided by the gem object (use the gem-provided global H2O
index method/field instead of computing numSpeciesInPhase_i(aq_index)-1), read
H2Oamount via gem.speciesAmounts()[globalH2OIndex] and H2Ommass via
gem.speciesMolarMasses()[globalH2OIndex], compute H2Omass = H2Oamount * H2Ommass
(do not divide by 1000 because speciesMolarMasses() is in kg/mol), and then
compute molality using gem.elementAmountsInPhase_i(aq_index) divided by that
H2Omass when populating out[gem.elementName_i(i)].
---
Outside diff comments:
In `@python/src/xgems/pyxGEMS.hpp`:
- Around line 791-806: The pybind11 binding for ChemicalEngine::setPT currently
exposes arguments as py::arg("temperature"), py::arg("pressure") but the native
signature is (P, T); swap the argument order and names to py::arg("pressure"),
py::arg("temperature") so positional and keyword calls map correctly, and update
the docstring parameter names and descriptions to list pressure first then
temperature to match ChemicalEngine::setPT.
In `@xGEMS/ChemicalEngineMaps.cpp`:
- Around line 163-169: The loop currently iterates all species (nspecies()) and
divides each amount by the aqueous volume, producing aqueous concentrations for
non-aqueous species; change it to iterate only the aqueous-phase species slice
(use the existing aq_species_molality()/or equivalent accessor) instead of
nspecies(), and compute molarity by dividing only those aqueous-species moles
(from moles_species or the aq slice) by gem.phaseVolume_i(aq_index)*1000; update
the keys using gem.speciesName_i(i) for the aqueous-species indices and leave
non-aqueous species out of ValuesMap.
---
Nitpick comments:
In `@xGEMS/ChemicalEngine.cpp`:
- Around line 484-497: The trailing comment in ChemicalEngine::indexSpecies
incorrectly says "element" when it should say "species"; update the comment on
the return statement (inside function ChemicalEngine::indexSpecies) to read
something like "in case that species name was not found" so it accurately
reflects that the function looks up species (and not elements).
- Around line 920-927: In ChemicalEngine::chemicalPotentials(), remove the
commented-out dead code "//return Vector{};" so the function body only assigns
pimpl->chemPotentials via pimpl->node->Get_muDC(i, false) and returns
pimpl->chemPotentials; locate the method by name
(ChemicalEngine::chemicalPotentials) and the members pimpl->chemPotentials and
pimpl->node->Get_muDC to safely delete that leftover comment.
In `@xGEMS/ChemicalEngineMaps.hpp`:
- Line 143: The Doxygen `@param` tags incorrectly use "@param optional phase ..."
— update each occurrence to "@param phase ..." and move any "optional" wording
into the descriptive text (e.g., "phase (optional): ..." or "If omitted,
defaults to the first index") so the param name is a valid Doxygen identifier;
apply this fix for the instances around the current diff and the additional
occurrences noted (near lines ~742-743, ~834-835, ~849-850, ~918-919, ~977)
ensuring the parameter name 'phase' remains consistent in comments for the
associated functions/classes in ChemicalEngineMaps.hpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 268f53f3-65d0-405d-b120-38fce55cfcdb
📒 Files selected for processing (17)
demos/demo.prodemos/demo1-dict.cppdemos/demo2.pydemos/resources/solvus2/series1-dat.lstdemos/resources/solvus2/series1-dat.lst.txtdemos/resources/solvus2/series1-dbr-0-0000.jsondemos/resources/solvus2/series1-dbr.lstdemos/resources/solvus2/series1-dch.jsondemos/resources/solvus2/series1-ipm.jsonenvironment.devenv.ymlinstall-dependencies.shpython/src/xgems/pyxGEMS.hpppython/src/xgems/pyxdGEMS.hppxGEMS/ChemicalEngine.cppxGEMS/ChemicalEngine.hppxGEMS/ChemicalEngineMaps.cppxGEMS/ChemicalEngineMaps.hpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
demos/demo2.py (1)
44-47: Remove trailing semicolons to satisfy linting.Lines 44 and 46 end with unnecessary semicolons (Ruff E703).
Suggested fix
-iphase1= engine.indexPhase("Kaolinite_long_phase_name"); +iphase1= engine.indexPhase("Kaolinite_long_phase_name") print(iphase1, " ", engine.phaseName(iphase1)) -iphase2= engine.indexPhase("Kaolinite"); +iphase2= engine.indexPhase("Kaolinite") print(iphase2, " ", engine.phaseName(iphase2))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/demo2.py` around lines 44 - 47, Remove the trailing semicolons at the end of the statements that call engine.indexPhase and engine.phaseName (look for the lines using indexPhase("Kaolinite_long_phase_name") and indexPhase("Kaolinite") assigning to iphase1 and iphase2) so the statements end without ";" to satisfy the Ruff E703 lint rule; update the two lines where iphase1 and iphase2 are assigned/printed to drop the semicolons.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/demo2.py`:
- Line 24: The demo uses a relative path in the ChemicalEngine constructor
(engine = ChemicalEngine("resources/solvus2/series1-dat.lst")) which breaks if
the working directory differs; change it to build an absolute path anchored to
the script using __file__ (e.g., resolve Path(__file__).parent /
"resources/solvus2/series1-dat.lst") and pass that path (as a string or Path) to
ChemicalEngine so the demo loads reliably regardless of CWD.
- Around line 18-19: The file currently uses wildcard imports "from xgems import
*" and "from numpy import *", which hides symbol origins and triggers Ruff F403;
replace them with explicit imports by inspecting which functions/classes are
actually used and either import those names (e.g., "from xgems import MyClass,
helper_fn") or import the modules with aliases (e.g., "import xgems" or "import
numpy as np") and update usages accordingly so all references clearly come from
xgems or numpy rather than star imports.
---
Nitpick comments:
In `@demos/demo2.py`:
- Around line 44-47: Remove the trailing semicolons at the end of the statements
that call engine.indexPhase and engine.phaseName (look for the lines using
indexPhase("Kaolinite_long_phase_name") and indexPhase("Kaolinite") assigning to
iphase1 and iphase2) so the statements end without ";" to satisfy the Ruff E703
lint rule; update the two lines where iphase1 and iphase2 are assigned/printed
to drop the semicolons.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
xGEMS/ChemicalEngineMaps.cpp (1)
148-151:⚠️ Potential issue | 🔴 CriticalH2O index calculation uses phase-local offset instead of global index.
numSpeciesInPhase_i(aq_index) - 1returns(count - 1), which is only valid as a global index if the aqueous phase starts at species index 0. For other phase orderings, this retrieves the wrong species. Compare to lines 181-182 which correctly compute the global index.🐛 Proposed fix
- auto H2Oindex = gem.numSpeciesInPhase_i(aq_index)-1; + auto H2Oindex = gem.indexFirstSpeciesInPhase_i(aq_index) + + gem.numSpeciesInPhase_i(aq_index) - 1; auto H2Oamount = gem.speciesAmount(H2Oindex);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xGEMS/ChemicalEngineMaps.cpp` around lines 148 - 151, The code uses a phase-local index for H2O (H2Oindex = gem.numSpeciesInPhase_i(aq_index)-1) which yields the wrong global species index; replace it by computing the global index using the phase start offset, e.g. H2Oindex = gem.firstSpeciesInPhase_i(aq_index) + gem.numSpeciesInPhase_i(aq_index) - 1 (or the equivalent API that returns the first/global index of the aqueous phase), then continue to use gem.speciesAmount(H2Oindex) and gem.speciesMolarMasses()[H2Oindex] to compute H2Omass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@xGEMS/ChemicalEngineMaps.cpp`:
- Around line 148-151: The code uses a phase-local index for H2O (H2Oindex =
gem.numSpeciesInPhase_i(aq_index)-1) which yields the wrong global species
index; replace it by computing the global index using the phase start offset,
e.g. H2Oindex = gem.firstSpeciesInPhase_i(aq_index) +
gem.numSpeciesInPhase_i(aq_index) - 1 (or the equivalent API that returns the
first/global index of the aqueous phase), then continue to use
gem.speciesAmount(H2Oindex) and gem.speciesMolarMasses()[H2Oindex] to compute
H2Omass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30ae7099-2b17-4565-b11f-3e4338a959a0
📒 Files selected for processing (3)
xGEMS/ChemicalEngine.cppxGEMS/ChemicalEngine.hppxGEMS/ChemicalEngineMaps.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- xGEMS/ChemicalEngine.cpp
- xGEMS/ChemicalEngine.hpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xGEMS/ChemicalEngineMaps.cpp`:
- Around line 447-460: In add_species_amt, the code uses the user-supplied
species string for map lookups (m_species_molar_mass, m_species_molar_volumes)
but gem.indexSpecies may resolve to a canonical name; fetch the
resolved/canonical name via gem.speciesName_i(species_idx) and use that key for
the map accesses instead of the incoming species parameter, and also guard
against zero-valued molar mass/volume before dividing to avoid divide-by-zero
(references: add_species_amt, gem.indexSpecies, gem.speciesName_i,
m_species_molar_mass, m_species_molar_volumes, nspecies(), b_amounts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78ddb519-40a6-417b-ba94-4a2b21bc154c
📒 Files selected for processing (1)
xGEMS/ChemicalEngineMaps.cpp
gdmiron
approved these changes
Apr 27, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Improvements
Chores