Hydraulic library updated to use macros #363
Conversation
Changes include: - sources.jl - MassFlow(; name, p_int) - FixedPressure(; p, name) - Pressure(; name)
Changes include: - sources.jl - MassFlow(; name, p_int) - FixedPressure(; p, name) - Pressure(; name) - components.jl - Cap(; p_int, name) - Open(; p_int, name) -
Only non-breaking changes were made. Changes include: - sources.jl - MassFlow - FixedPressure - Pressure - components.jl - Cap - Open - FlowDivider - VolumeBase - FixedVolume - Volume The following were NOT changed, as the changes would be breaking: - components.jl - TubeBase - Tube - ValveBase - Valve - DynamicVolume - SpoolValve - SpoolValve2Way - Actuator
|
Yes; you could just include all the changed in 367 to this PR or if that merges earlier, rebase this onto the new main |
| @parameters begin | ||
| end | ||
|
|
||
| @variables begin | ||
| end |
There was a problem hiding this comment.
These are not required; mtkmodel handles that robustly.
| @variables begin | ||
| end |
|
|
||
| @parameters begin | ||
| end | ||
|
|
||
| systems = @named begin | ||
| @variables begin | ||
| end |
.gitignore
Outdated
| @@ -21,4 +21,4 @@ docs/site/ | |||
| # It records a fixed state of all packages used by the project. As such, it should not be | |||
| # committed for packages, but should be committed for applications that require a static | |||
| # environment. | |||
| Manifest.toml | |||
| Manifest.toml | |||
There was a problem hiding this comment.
Needs a new line at the end (to keep Git-diff rendering happy).
| @parameters begin | ||
| end |
There was a problem hiding this comment.
Can be dropped. @parameters and @variable blocks aren't compulsory; mtkmodel handles such cases robustly.
| systems = @named begin | ||
| port = HydraulicPort(;) | ||
| @parameters begin | ||
| Volume |
| dm = port.dm | ||
| p = port.p |
There was a problem hiding this comment.
This can be added to a
begin
...
end
(outside @equations)
As dm and p are not defined as variables, these equations will throw errors.
There was a problem hiding this comment.
Looks like accessing values from subcomponent can't be added to begin...end. My bad.
Alternative fix would be, define dm and p as @variables.
And move these back to @equations .
There was a problem hiding this comment.
This seems simplest?
@equations begin
D(rho) ~ drho
rho ~ full_density(port, port.p)
port.dm ~ drho * vol
end| area = area | ||
| @parameters begin | ||
| area | ||
| direction = +1 |
There was a problem hiding this comment.
direction could be of type @structurtal_parameters.
Is this intentionally promoted to a @parameters?
While here, could you change +1 to 1?
|
|
||
| ODESystem(eqs, t, vars, pars; name, systems, defaults = [rho => liquid_density(port)]) |
There was a problem hiding this comment.
defaults = [rho => liquid_density(port)]
can be added via the @defaults block https://docs.sciml.ai/ModelingToolkit/stable/basics/MTKLanguage/#@defaults-begin-block
There was a problem hiding this comment.
Is this correct?
@defaults begin
rho => liquid_density(port)
end| x1 = 1 | ||
| x2 = 1 |
There was a problem hiding this comment.
x1 x2 were @structural_parameters. These could be just @parameters. However, that would need fixing these scoping here: https://github.com/SciML/ModelingToolkitStandardLibrary.jl/blob/main/src/Hydraulic/IsothermalCompressible/components.jl#L648-L650 in DynamicVolume
Probably adding them as structural_parameters is easier.
@ven-k comments implemented. Everything exactly as you said. Only thing I am unsure about is default, which is implemented as follows: |
|
Thanks for all the updates. I see that there are few Hydraulic test failures.
|
Apologies - I should have tested this locally. I would have never ever realized that X is different to symbol(X). Thanks @ven-k for pointing this out. |
|
@ven-k thanks for the input. I apologize, it's 100% breaking in the tests. I'll need to take my time to fix and debug. |
Update based on documentation build error fixed
|
@ven-k back to 572 tests passed |
|
@ven-k Thanks for all the input - much appreciated. I believe it is ready now for review? |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Reviewer @ven-k comments implemented
The models in the Hydraulic library were modified to use @mtkmodel macro instead of @component function. The functionality of the models should remain the same.
Only non-breaking changes were made. Changes include:
The following were NOT changed, as the changes would be breaking:
Additionally, the docstrings were altered to remove P_int and its derivatives, which are no longer used.
The package was tested and only the original fail of VariableResistor occurs.