Skip to content
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

Add first draft of stock and flow schema and example #61

Merged
merged 2 commits into from
Oct 18, 2023
Merged

Conversation

bgyori
Copy link
Collaborator

@bgyori bgyori commented Oct 6, 2023

This is a proposed stock and flow AMR which reuses as many components as possible from existing AMRs while following the overall structure of the stock and flow ACSet.

Co-authored-by: Brandon Rose <[email protected]>
Co-authored-by: liunelson <[email protected]>
Co-authored-by: Matthew Printz <[email protected]>
@brandomr brandomr self-requested a review October 10, 2023 16:01
Copy link
Collaborator

@brandomr brandomr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

@mattprintz
Copy link
Contributor

Question: How do we represent a flow that upstreams or downstreams from/to a "cloud" rather than a defined stock?

@bgyori
Copy link
Collaborator Author

bgyori commented Oct 10, 2023

Question: How do we represent a flow that upstreams or downstreams from/to a "cloud" rather than a defined stock?

I think the placeholder for the "cloud" would just be left empty, i.e., upsteram_stock: null.

Comment on lines +130 to +131
"links": {
"type": "array",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can foresee us needed to include additional information on the links, such as "color"/"category", "polarity", "label", etc.
I am not sure which, if any, of these should be included in the schema or if we should just include a meta/extras property to store any of this extra information.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put any of those keys under an optional properties block.

@enoriega
Copy link
Contributor

@brandomr @bgyori How stable is the amr's schema for stock and flow models?
What particular elements of such AMR are candidates for linking with extractions? I am looking at:

  • flows
  • stocks
  • auxiliaries
  • ode parameters

Does it make sense to link those classes of elements?

@bgyori
Copy link
Collaborator Author

bgyori commented Oct 18, 2023

@brandomr @bgyori How stable is the amr's schema for stock and flow models? What particular elements of such AMR are candidates for linking with extractions? I am looking at:

  • flows
  • stocks
  • auxiliaries
  • ode parameters

Does it make sense to link those classes of elements?

That sounds right. In terms of the schema: there haven't been any further comments since I posted this 2 weeks ago to request changes so I think we'll go with what we have here.

@@ -1,7 +1,7 @@
{
"header": {
"name": "SIR Model",
"schema": "",
"schema": "https://raw.githubusercontent.com/DARPA-ASKEM/Model-Representations/stock-flow/stockflow/stockflow_schema.json",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good temporary addition, thanks! We will ideally change this to a tagged version URL once the PR is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwdchang pointed this out so I made this change so we pass all the checks for merger!

@liunelson liunelson merged commit ae12135 into main Oct 18, 2023
5 checks passed
@liunelson liunelson deleted the stock-flow branch October 18, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants