-
Notifications
You must be signed in to change notification settings - Fork 1
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
Rewrite #64
Rewrite #64
Conversation
paramvars = [only(@parameters $x) for x in paramnames] | ||
paramvals = [x.value for x in ode.parameters] | ||
sym_defs = paramvars .=> paramvals | ||
initial_exprs = [MathML.parse_str(x.expression_mathml) for x in ode.initials] |
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.
@jpfairbanks this is where we use the MathML, but we could just symbolics parse the other expressions.
Okay, still needs to get the calibrate endpoint back online to achieve parity with previous version, but other than that I think this is good to go. I'll add it back in tomorrow morning (as well as dealing with the merge with main). |
I still need to bring calibrate online, but other than that, I think we're good to go. For reviewers, the things that need the most scrutiny are functions that contain a mention of
|
We want to completely change the calibrate endpoint anyways, so I'd say it's fine merging without it |
|
||
#-----------------------------------------------------------------------------# calibrate | ||
struct Calibrate <: Operation | ||
# TODO |
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.
the main thing I'd need to know is what exactly would be in here.
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.
Anything you want. It just needs to have a Calibrate(o::OperationRequest)
method.
timespan::Tuple{Float64, Float64} | ||
end | ||
|
||
Simulate(o::OperationRequest) = Simulate(ode_system_from_amr(o.model), o.timespan) |
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.
out of curiosity, where it is known that OperationRequest has o.model and o.timespan?
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.
I'm not sure I follow. Those are fields of the struct.
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.
Check examples/
to see what the incoming requests look like. E.g.
{
"model_config_id": "f20ea672-bfa4-422b-884f-0a4c09356fd0",
"timespan": {"start": 0, "end": 90}
}
We then reach out to the TDS to get the the AMR (model
) from the given model_config_id
.
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.
But that all happens within the OperationRequest
constructor
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #64 +/- ##
===========================================
+ Coverage 23.45% 72.19% +48.73%
===========================================
Files 12 1 -11
Lines 324 187 -137
===========================================
+ Hits 76 135 +59
+ Misses 248 52 -196
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
As part of trying to understand the code better, I ended up rewriting it 😅.
My goals were:
The whole package is currently a single ~350-line file: SimulationService.jl. I'd expect this to grow/split into more files as needed in the future. For now it's nice for searchability and getting new people involved in the code.
TODOs for parity with current version
These should be easy lifts, using the
Simulate
code as a template. For example, here is all of the simulate-specific code: