Add PropsSImulti function - #51
Conversation
* Implements the `PropsSImulti` function using `ccall` on the appropriate function from the CoolProp C-interface. * Overloads `PropsSI.()` broadcasting to internally use `PropsSImulti`. This avoids the `AbstractState` allocation overhead for each state pair. * Adds test for `PropsSImulti` and `PropsSI.()` calls. Fix CoolProp#22
📝 WalkthroughWalkthroughAdds ChangesPropsSImulti Implementation
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
I have tested this on Linux x64 and Windows x64. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/runtests.jl (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for scalar
value1+ vectorvalue2and for multiple outputs.Both new assertions pass a vector as
value1and a scalar asvalue2, so they miss thebroadcastedbug (seesrc/CoolProp.jlLine 212) and the multi-output reshape path. A case likePropsSI.("D", "T", 300, "P", [101325, 101325], "R125[0.5]&R32[0.5]")and a"D,H"PropsSImultiassertion would guard both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtests.jl` around lines 48 - 50, The current tests in PropsSI/PropsSImulti only cover vector value1 with scalar value2, so they do not exercise the broadcasted path in PropsSI or the multi-output reshape behavior in PropsSImulti. Add a PropsSI. test that uses scalar value1 and vector value2, and add a PropsSImulti assertion with multiple outputs (for example including both D and H) to verify the reshape logic and catch the broadcasted bug in CoolProp.jl’s PropsSI handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/CoolProp.jl`:
- Around line 208-223: The `broadcasted(::typeof(PropsSI), ...)` method computes
`size_values` from `value1` twice, so scalar `value1` with vector `value2` gets
the wrong broadcast size. Fix the size calculation to use both inputs’ lengths
in `broadcasted` before normalizing `value1` and `value2`, ensuring the scalar
is expanded to match the vector and the arguments passed to `PropsSImulti` stay
the same length. Update the logic in `broadcasted` so it correctly handles
either side being the scalar.
---
Nitpick comments:
In `@test/runtests.jl`:
- Around line 48-50: The current tests in PropsSI/PropsSImulti only cover vector
value1 with scalar value2, so they do not exercise the broadcasted path in
PropsSI or the multi-output reshape behavior in PropsSImulti. Add a PropsSI.
test that uses scalar value1 and vector value2, and add a PropsSImulti assertion
with multiple outputs (for example including both D and H) to verify the reshape
logic and catch the broadcasted bug in CoolProp.jl’s PropsSI handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6ce669b-a93f-40f2-8d06-65ccaa60caf4
📒 Files selected for processing (2)
src/CoolProp.jltest/runtests.jl
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/runtests.jl (1)
49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
"backend::fluid"notation inPropsSI.broadcast.The
broadcastedmethod insrc/CoolProp.jlexplicitly handles theoccursin("::", fluid)case to extract a custom backend, but none of the new tests exercise this path. All broadcast tests use the default"HEOS"backend. Adding a test likePropsSI.("D", "T", [300, 350], "P", 101325, "REFPROP::R125[0.5]&R32[0.5]")would validate the backend extraction logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/runtests.jl` around lines 49 - 52, Add a broadcast test in test/runtests.jl that exercises PropsSI. with a fluid string using the "backend::fluid" form so the broadcasted path in src/CoolProp.jl that checks occursin("::", fluid) is covered. Use a case like PropsSI. over temperature/pressure inputs with "REFPROP::R125[0.5]&R32[0.5]" and assert the expected density results, alongside the existing PropsSI. tests that currently only cover the default HEOS backend.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/runtests.jl`:
- Around line 49-52: Add a broadcast test in test/runtests.jl that exercises
PropsSI. with a fluid string using the "backend::fluid" form so the broadcasted
path in src/CoolProp.jl that checks occursin("::", fluid) is covered. Use a case
like PropsSI. over temperature/pressure inputs with
"REFPROP::R125[0.5]&R32[0.5]" and assert the expected density results, alongside
the existing PropsSI. tests that currently only cover the default HEOS backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bb5c0e6-fa59-44e7-9e2c-936232399f34
📒 Files selected for processing (2)
src/CoolProp.jltest/runtests.jl
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CoolProp.jl
PropsSImultifunction usingccallon the appropriate function from the CoolProp C-interface. Thus, most of the work ist done by CoolProp, e.g. returningInffor single value pairs with calculation errors and extraction of fractions from the fluid string.PropsSI.()broadcasting to internally usePropsSImulti. This avoids theAbstractStateallocation overhead for each state pair and allows to usePropsSImultiwith thePropsSIcall signature like in the Python wrapper.PropsSImultiandPropsSI.()calls.Fix #22
Summary by CodeRabbit
New Features
Tests