-
Notifications
You must be signed in to change notification settings - Fork 52
First draft next_state rewrite [Closes #528] #529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thank you for opening this PR. Each PR into dev requires a code review. For the code review, look at the following:
|
Benchmarking Results
|
Codecov Report
@@ Coverage Diff @@
## dev #529 +/- ##
==========================================
+ Coverage 79.83% 80.28% +0.45%
==========================================
Files 30 30
Lines 2405 2394 -11
==========================================
+ Hits 1920 1922 +2
+ Misses 485 472 -13
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
093a4ae
to
b21e87f
Compare
Benchmarking Results [Update]
To:
|
Benchmarking Results [Update]
To:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Clever way to implement this. You don't have to do anything for this PR with this comment, but I was browsing the centrifugal pump model and it looks like it is considered a discrete model because of the QLeak state but all the other hidden states are continuous. I'm curious if you have any thoughts on how these integration methods could be utilized in a hybrid model, or if it should just be left up to the person writing their model including the integration method in their next state function.
Benchmarking Results [Update]
To:
|
Thanks @MikeAndSpencer - This is a good question. I don't have a good solution now, but I just opened an issue to explore it more. |
Update integration logic to apply not just in simulation but also directly to the next_state method. Solution developed in collaboration with partners at NGC