Repository navigation
prepare solver and cli interfaces for co-simulation - #635
superwhiskers wants to merge 1 commit into
Conversation
621e39a to
7d7a0cd
Compare
lukelowry
left a comment
There was a problem hiding this comment.
Some initial thoughts. Also, I don't know much about .nix, could you give some info? Looks like good architecture changes overall
| const int consistentICType = getIDAConsistentICType(); | ||
| retval = IDACalcIC(solver_, consistentICType, t0 + 0.1); | ||
|
|
||
| retval = IDACalcIC(solver_, consistentICType, t0 + 0.1); |
There was a problem hiding this comment.
Out of scope of PR, but why is this arbitrarily + 0.1? Could this be related to convergence issues post-fault? @Steven-Roberts
There was a problem hiding this comment.
The argument should be
the first value of 𝑡 at which a solution will be requested (from IDASolve()). This value is needed here only to determine the direction of integration and rough scale in the independent variable 𝑡.
Since we don't have access to tf yet, I guess + 0.1 was arbitrarily picked. The convergence issues typically occur a couple steps after IDACalcIC is called, we I don't think this is the culprit. Nevertheless, I can check and see if another value is more principled here.
| int getDefaultInitialCondition(); | ||
| int initializeSimulation(RealT t0, bool findConsistent = true); | ||
| int initializeSimulation( | ||
| RealT t0, bool findConsistent = true, bool printMonitor = true); |
There was a problem hiding this comment.
This feels like an unfortunate place to add printMonitor, thoughts?
7d7a0cd to
e024dec
Compare
this was only in here initially due to the patched enzyme to make using it easier for others. i've left it in here as it's a harmless change so i can stop maintaining the environment out of tree, but it can be removed if necessary. as it's used here, it's just a way of describing how to build gridkit / set up an environment for developing it. if you want more information than that / want to try to use it, you can learn more here. if you have access to the excl cluster, a handful of the machines there have it |
d634f3f to
fb6dbde
Compare
Perhaps move the relevant nix files to |
conventionally additionally, if it weren't at the root, then someone wanting to pull in gridkit as an input to a flake of their own would need to override the directory it is in.
and to clarify, you can use nix on any machine you want, i only mentioned excl as it's already installed on some of the machines there. if you don't have access or want to use it on a personal machine it's something you could freely install yourself as i have |
fb6dbde to
31d28cc
Compare
- add a nix development environment - extend cli parsing to support positional arguments - extend kinsol with callback-based solves - improve sundials interface error handling and monitor control - add fault support to signal voltage output buses - support external signal bindings in jacobian tests
31d28cc to
866f332
Compare
pelesh
left a comment
There was a problem hiding this comment.
The proposed changes here need more discussion and a dedicated issue(s).
A few obvious suggestions:
- Buses wired to signals should not be faulted. Adding fault support for those is a distraction at this point.
- No changes to
Idashould be made in this PR. Most of the changes toIdaas I see are formatting changes and these must be reverted before the review. - If changes to
Kinsolare required, they need to be discussed in a separate issue. - All flake related configuration needs to be removed. We are using Spack for packaging, so any configuration adjustments should be contributed there.
| virtual int setFault(bool status, RealT R, RealT X) override final; | ||
| virtual int allocate() override final; |
There was a problem hiding this comment.
I suggest we make busses with associated signals "unfaultable". When you dedicate a bus as a co-simulation connection point, you typically don't want to fault it.
| virtual int setFault(bool status, RealT R, RealT X) override final; | |
| virtual int allocate() override final; | |
| virtual int allocate() override final; |
There was a problem hiding this comment.
Similarly, remove fault support for signal connected buses elsewhere in the code.
Description
prepares the solver and cli interfaces for co-simulation work
Proposed changes
there's a small amount of diff noise here from breaking this out of the earlier monolithic commit which included formatting changes. that can be reduced further if necessary
Checklist
-Wall -Wpedantic -Wconversion -Wextra.