Add Schemas for Stock and Flow - #35
jpfairbanks wants to merge 11 commits into
Conversation
not entirely sure what some of the tables are, need to update
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
=======================================
Coverage 87.07% 87.07%
=======================================
Files 6 6
Lines 325 325
Branches 54 54
=======================================
Hits 283 283
Misses 36 36
Partials 6 6
☔ View full report in Codecov by Sentry. |
|
Here is the example And in JSON {
"Flow": [
{
"_id": 1,
"u": 1,
"d": 2,
"fname": "NewIncidence",
"ϕf": "p.cβ*u.S*u.I/p.N"
},
{
"_id": 2,
"u": 2,
"d": 3,
"fname": "NewRecovery",
"ϕf": "(u.I/p.tr)"
}
],
"Stock": [
{
"_id": 1,
"sname": "S"
},
{
"_id": 2,
"sname": "I"
},
{
"_id": 3,
"sname": "R"
}
],
"Link": [
{
"_id": 1,
"s": 1,
"t": 1
},
{
"_id": 2,
"s": 2,
"t": 1
},
{
"_id": 3,
"s": 2,
"t": 2
}
],
"Name": [
],
"FuncFlow": [
]
} |
|
I've done the following:
Unfortunately, it's never so easy. The validator is giving the following feedback: I guess this means we need to explicitly specify all of the parts of the schema, even if they're empty lists. It might be possible to make all fields default to an empty list but I don't want to conflate two different lines of work in this PR |
|
I was able to update the example to fix the names and include the missing fields - note @jpfairbanks's example had "Stock" and "Flow" as the keys but the schema was looking for "S" and "F". I changed them manually to make it work, but this might indicate there's something else funny going on |
| ob.name, | ||
| **{ | ||
| prop.name: (Union[self.valtype(prop), None], None) | ||
| prop.name: (Union[self.valtype(prop), None], Field(title=ob.title, description=ob.description)) |
There was a problem hiding this comment.
is it the case that all elements will always be lists? If that's the case, we can use this line to introduce default empty lists.
| "name": "V" | ||
| }, | ||
| { | ||
| "title": "Linkage Variable?", |
There was a problem hiding this comment.
There are some question marks here we probably want to address
|
I think the two example I posted is an example of the primitive schema which uses Stock and Flow for the table names. In the extended schema the tables are abbreviated (because there are so many more of them). The example was right, but it was for the StockFlowp.json schema. I didn't post the corresponding example for the StockFlow.json schema. |
|
Oh I see. Then we should merge them together and denote which parts are optional |
|
"Optional Tables" doesn't really fit with Catlab's model of schemas. Tables can be empty, but that doesn't make the table optional. Catlab has a notion of schema inheritance where a schema can extend another schema by adding tables or fields. I guess the longer term right thing to do is make StockFlow.jl use that inheritance mechanism, but that would require updating library code in StockFlow and user code. It would be a breaking change to the API of that repo, because it requires changing the field names. I think keeping them separate for now and just validating models against the correct schema is the easier move at this point. |
|
Okay so then my understanding is that you have some fields but don’t necessarily have to use all of them. So is it correct that one of these schemas is a proper superset of the other? Then we can just keep the more expressive one, right? We won’t need two AMRs |
Well that depends on how you interpret "use". If a database instance The broader Catlab view is that relationships between schemas also introduces relationships between instances over those schemas. For example, If
The way that it is implemented, neither schema is a proper superset of the other for two reasons.
Then we can just keep the more expressive one, right? There is a key conceptual difference between them. The In the
I think that we need both formats. If you want to store your flow functions as a black box primitive function, you should use If you want to start with one format at first, then you should do the |
| { | ||
| "title": "Flow Function", | ||
| "description": null, | ||
| "name": "ϕf", |
There was a problem hiding this comment.
Are there any potential integration issues with using a less-common unicode character as a JSON key?
I'm also wondering if this might be especially an issue since there are two unicode glyphs for phi, U+03C6 and U+03D5.
While this is definitely the convention in Julia, perhaps for the interchange format it could be something that is potentially more universally accessible such as ffunc or flowfunc?
There was a problem hiding this comment.
We should test the JSON libraries. This would be a breaking change to StockFlow.jl so let's find an upstream bug before we patch it?
There was a problem hiding this comment.
Ah. I didn't realize that this would be a breaking change in the other library.
According to the JSON spec, it should be fine, and it obviously works in Python and JS, so I'm happy to assume that everything conforms to the spec until proven otherwise.
There was a problem hiding this comment.
Yeah the SF.jl library already exists and I was trying to get this in to py-acsets with the minimal code change possible. So far we haven't made any other changes to SF.jl.
|
I got some feedback from the StockFlow.jl team on the names for the full-fledged schema AlgebraicJulia/StockFlow.jl#92 (comment). A nice hand drawn schema with longer names. |
|
Ok, digging into the full-fledged stock flow theory, I definitely think we should just start with |
|
Hey all, I'd like to jump into this conversation to express some comments about the StockFlow(p) schema. A minor issue is that the proposed schema deviates from some of the conventions specified in the Model Representation schemas:
Also, is the intention to have a section ( Otherwise, might the above points be addressed within the Model-Representation schema and outside of the AlgebraicJulia py-acsets schema? |
|
I'm happy to merge this once it's been rebased; I shouldn't have let it languish so long. |
Not entirely sure what some of the tables are, need to update once I talk to @Xiaoyan-Li.