TS-CUBED / CirculatorySystemModels.jl

MIT License
16 stars 5 forks source link

Chamber has undefined 'DE' #23

Closed justidy1 closed 9 months ago

justidy1 commented 9 months ago

When I run the following code, the following error occurs

@named sin_chamber = Chamber(fun=sin)

OUTPUT:

ERROR: UndefVarError:DEnot defined Stacktrace: [1] macro expansion @ ~/.julia/packages/CirculatorySystemModels/g2qCJ/src/CirculatorySystemModels.jl:453 [inlined] [2] (::CirculatorySystemModels.var"#50#51"{typeof(sin)})() @ CirculatorySystemModels ~/.julia/packages/ModelingToolkit/MDLZQ/src/systems/abstractsystem.jl:1172 [3] Chamber(; name::Symbol, V₀::Float64, E::Float64, fun::typeof(sin)) @ CirculatorySystemModels ~/.julia/packages/ModelingToolkit/MDLZQ/src/systems/abstractsystem.jl:1172 [4] top-level scope @ ~/.julia/packages/ModelingToolkit/MDLZQ/src/systems/abstractsystem.jl:1006

The error seems to be in the source code for Chamber, which has no definition for DE.

TS-CUBED commented 9 months ago

Thanks for this. The Chamber was indeed defined in terms of "p" still and only half refactored in terms of "V".

Fixed this in "main" - could you test it for your case?

Note: rejected your PR, since it was in terms of "p" and was missing the correct scaling factor for E in D(fun(t)).

justidy1 commented 9 months ago

Hi, I just tried this fix, but now it seems to be missing a p_0 term. I have opened a new PR with the proposed fix.

TS-CUBED commented 9 months ago

Oops. That p_0 shouldn't be there, actually. It's just a pressure offset that Shi uses in their paper. A perfect chamber should not have that.

For consistency with Shi we should include it as a parameter if we use it. But for a simple Chamber, we may just delete it completely.

justidy1 commented 9 months ago

Great, thank you for these quick fixes! I appreciate your help.

TS-CUBED commented 9 months ago

Hello justidy1,

I have released version 0.3.0, which includes all of these. Have a look at the new VariableElastance which should be a drop-in replacement for the Chamber, which has been removed. The new element has a few more features, but with the defaults it should just behave the same as the old chamber, but be formulated in terms of $dV/dt$, rather than the old form in $dp/dt$. In the DHChamber this new form proved to be an order of magnitude faster in the compute times.