WIP update to use InterTypes #29

p-stokes · 2023-12-01T15:34:27Z

This is to make the InterType form of the various structures currently in SyntacticModels.

Resolves #28

… a corresponding test script intertype_examples.jl

codecov · 2023-12-01T15:45:42Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (0ea6f64) 81.74% compared to head (e07fb29) 87.11%.

Files	Patch %	Lines
src/amr.jl	86.44%	8 Missing ⚠️
src/decapodes.jl	96.73%	5 Missing ⚠️
src/composite_models.jl	87.50%	2 Missing ⚠️
src/uwd.jl	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   81.74%   87.11%   +5.37%     
==========================================
  Files           6        6              
  Lines         378      489     +111     
==========================================
+ Hits          309      426     +117     
+ Misses         69       63       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…amr.it version. Changed amr module name to AMR.

…ith jsonread and jsonwrite

…ted the uwd intertypes.

…model and readback in core_it.jl. readback currently won't work because intertypes parsing doesn't recognize the typeof variant sum types.

…. Can now construct from an intertype version of DecaExpr. Still need to add SummationDecapode schema so it can be a field type for an ASKEMDecapode.

…type module but the SummationDecapode formation function is not yet producing an intertype one.

…r uses Decapodes.jl. Copied in a few utilities from there. Edited readback function in core_it.jl

…pdating the module and runtest files accordingly. Had to change the amr.it name within uwd.it from AMR to amr.

…me to amr. Changed my_parse_decapode function name as well.

jpfairbanks

The unnecessary rename of these file makes the diffs unreviewable. Please put them back and confirm that the diffs are useful. I can review again at that time.

jpfairbanks · 2023-12-06T16:42:05Z

Project.toml


 [compat]
 ACSets = "0.2"
 Catlab = "0.15, 0.16"
-Decapodes = "0.5"
+Decapodes = "0.4, 0.5"


Since code is moved here, do we still need to depend on Decapodes?

src/SyntacticModels.jl

jpfairbanks · 2023-12-06T16:44:16Z

src/amr_it.jl

@@ -0,0 +1,391 @@
+module AMR


because of the rename of this file, the diff is useless. See above comment.

jpfairbanks · 2023-12-06T16:44:28Z

src/amr_it.jl

+using ACSets.ACSetInterface
+using StructTypes
+
+# using ..SyntacticModelsBase


Delete commented out code

jpfairbanks · 2023-12-06T16:44:40Z

src/amr_it.jl

+  return Base.Meta.parse(distro_string(d))
+end
+
+#=


delete block comments.

jpfairbanks · 2023-12-06T16:45:33Z

src/composite_models_it.jl

@@ -0,0 +1,85 @@
+module Composites


again, I can't review this diff because of the file rename.

jpfairbanks · 2023-12-06T16:46:52Z

src/decapodes_it.jl

+using ..AMR
+
+using StructTypes
+# using Decapodes


is anything using Decapodes now that you copied code over here? Should we upgrade Decapodes to use InterTypes and then just use InterType inteheritance here?

jpfairbanks · 2023-12-06T16:48:08Z

src/decapodes_it.jl

+  return d
+end
+
+function recognize_types(d::decapodes.AbstractNamedDecapode)


Should SynacticModels be doing decapodes operations like recognize_types? What do you think the scope boundary is?

jpfairbanks · 2023-12-06T16:48:56Z

src/decapodes_it.jl

+    end
+end
+
+function parse_decapode(expr::Expr)


Should SynacticModels be doing decapodes operations like parse_decapode? What do you think the scope boundary is?

jpfairbanks · 2023-12-06T16:49:24Z

src/uwd_it.jl

@@ -0,0 +1,191 @@
+module ASKEMUWDs


I can't review this diff because the file was renamed unnecessarily.

… is not yet working; some possibly needed upstream Decapodes and Catlab functions are copied into src and parts of test are commented out.

…to finish the functionality in amr. Copied in commented version of old example code into test.

…riginal files for easier diff view.

…diff.

… files for difference viewing.

… diff view.

p-stokes · 2023-12-07T02:43:09Z

The unnecessary rename of these file makes the diffs unreviewable. Please put them back and confirm that the diffs are useful. I can review again at that time.

I've updated the file names for the PR, and the diffs look ok now. Regarding the included decapodes functions, I think the longer-term plan is for Decapodes and other structures to be defined directly upstream with InterTypes. But for the short-term, it seemed easier to develop locally within SyntacticModels.

I do think it would be helpful to have a discussion about the design/scope plan for SyntacticModels because it relates to the current points of development for the AMR and Composites functionality as well.

jpfairbanks · 2023-12-07T15:07:47Z

src/uwd.jl

-varname(v::Var) = @match v begin
-  Untyped(v) => v
-  Typed(v, t) => v
+varname(v::uwd.Var) = @match v begin


@p-stokes, is there a problem with doing

const Var = uwd.Var

and then not needing to do all these qualified names?

Even that might not really be necessary. It might be possible to just use Var in this case. I'll check.
The qualification of names was somewhat inconsistent as I was developing, so I do plan to go through and do a standardization once the functionality is set.

I've cleaned up the name qualifications.

jpfairbanks · 2023-12-07T15:08:31Z

test/composite_models_examples.jl

+using Test
+using OrderedCollections
+import JSON
+import JSON3


What functionality are you using from all these additional imports?

Similar to the name qualifications, I was a bit lax with the imports during development.
I have already begun going through and cleaning up some of the unnecessary imports.

I've cleaned up the imports.

jpfairbanks · 2023-12-07T15:08:55Z

test/composite_models_examples.jl




-h = AMR.Header("harmonic_oscillator",
+h = Header("","harmonic_oscillator",


The fields of the Header are changing?

In the Model-Representations repo, I noticed Five had added an extra id field to the Header there, so I incorporated it here. I can go through and take it out if you want?

That upgrade for compatibility with Model-Representations should be a separate issue and PR because it is not related to "update to use InterTypes"

Ok. I'll undo that and make a separate issue and PR after.

I've removed the id field.

jpfairbanks · 2023-12-07T15:09:39Z

src/composite_models.jl


 using MLStyle
 using Catlab
-using Decapodes
+using Decapodes # : SummationDecapode


if you don't need it delete the import.

It was deleted in the imports cleanup.

…ved references to Decapodes.jl in decapodes.jl.

p-stokes · 2023-12-08T07:00:26Z

I cleaned up and standardized the imports and name qualifications. Still working on the remaining composite oapply example and amr acsetspec examples

jpfairbanks · 2023-12-08T14:13:16Z

I do think it would be helpful to have a discussion about the design/scope plan for SyntacticModels because it relates to the current points of development for the AMR and Composites functionality as well.

I think the first part of that discussion is understanding your expectations for the scope. What are your thoughts?

jpfairbanks · 2023-12-08T14:16:07Z

src/amr.it

+  element::Vector{Args}
+end
+
+#=


do we need this?

jpfairbanks · 2023-12-08T14:19:49Z

src/amr.it

@@ -0,0 +1,102 @@
+
+@sum Args begin


Is this being copied from https://github.com/AlgebraicJulia/ACSets.jl/blob/main/src/ADTs.jl#L14?

We should upgrade that code to use InterTypes and then import that usage here.

p-stokes · 2023-12-08T16:28:54Z

I do think it would be helpful to have a discussion about the design/scope plan for SyntacticModels because it relates to the current points of development for the AMR and Composites functionality as well.

I think the first part of that discussion is understanding your expectations for the scope. What are your thoughts?

Summarizing our in-person discussion about this:
The short-term scope is to convert the functionality and test examples to use InterTypes locally within SyntacticModels.jl (as best makes sense and is possible). Longer-term, the .it files and functions will be upstreamed to their respective repositories.
There are several blocking issues with InterTypes' capabilities and integration with some other upstream functionality (e.g., ADTs and Open). I will make issues for those and tag them below.

…e new PR to add later if needed.

…decapode to a Decapodes one, then Decapodes.Open can be used to compose. Still need to add reverse conversion.

… Updated remaining OpenDecapode function to oapply composite model and convert back to intertype.

… uses in examples. jsonwrite in first method still not working for empty annotations, so those examples are still commented.

… needed to make write_json_model and readback work.

…es in .it as needed. Load to AMR from expression examples now work. Need to update amr_to_string (and check other functions) to match current intertypes.

…eNode. Need to update load from dict for ASKEModel and Typing and amr_to_string.

…s of Strings for some examples. Also still need to fix amr_to_string and amr_to_expr.

…relates to intertype version of ACSetSpec is different format from ADTs. May want to convert back for expr or string. amr_to_string runs but doesn't look good.

olynch · 2024-01-17T21:01:01Z

Some of the work here ends up duplicating work merged into DiagrammaticEquations, so it makes sense to make a new PR

Started making versions with InterTypes, amr.it and decapodes.it, and…

883a465

… a corresponding test script intertype_examples.jl

p-stokes requested review from jpfairbanks and olynch December 1, 2023 15:34

p-stokes marked this pull request as draft December 1, 2023 15:34

p-stokes added 12 commits December 1, 2023 11:58

Fixed typos in decapodes.it and updated intertype_examples.jl

1c91145

Removed redundant Header def from decapodes.it and added id field to …

81d1d9c

…amr.it version. Changed amr module name to AMR.

Added decapode parsing functions and test example for ASKEMDecaExpr w…

6f3df6f

…ith jsonread and jsonwrite

Started setting up src files to export the intertype structures. Star…

84a37dc

…ted the uwd intertypes.

uwd write and read test example working. needed to change write_json_…

361199c

…model and readback in core_it.jl. readback currently won't work because intertypes parsing doesn't recognize the typeof variant sum types.

Added intertype version of SummationDecapode function and its helpers…

aa5cf1a

…. Can now construct from an intertype version of DecaExpr. Still need to add SummationDecapode schema so it can be a field type for an ASKEMDecapode.

Added intertype structs for SummationDecapode. It will load the inter…

cd83f9d

…type module but the SummationDecapode formation function is not yet producing an intertype one.

Added/updated functions to make intertype SummationDecapode. No longe…

692a242

…r uses Decapodes.jl. Copied in a few utilities from there. Edited readback function in core_it.jl

Started factoring amr and uwd parts into source and test files, and u…

2d073e3

…pdating the module and runtest files accordingly. Had to change the amr.it name within uwd.it from AMR to amr.

Updated Project.toml

a4b61af

Factored decapodes part into src and test. Again had to change AMR na…

c0a34fb

…me to amr. Changed my_parse_decapode function name as well.

Started making intertype version for composite_models.

f6e4db1

jpfairbanks requested changes Dec 6, 2023

View reviewed changes

p-stokes added 8 commits December 6, 2023 15:28

Updated composite_models_it src and test. Intertype version of oapply…

eaa5c91

… is not yet working; some possibly needed upstream Decapodes and Catlab functions are copied into src and parts of test are commented out.

Started working on intertype version of ACSetSpec and other ADT code …

aed4d45

…to finish the functionality in amr. Copied in commented version of old example code into test.

Moved amr_it.jl contents into amr.jl to better view differences.

744bbca

Moved contents of decapodes_it.jl and uwd_it.jl files to respective o…

d1ad500

…riginal files for easier diff view.

Moved contents of composite_models_it.jl to original file for easier …

8c4b047

…diff.

Moved contents of _it_ex.jl files to their corresponding _examples.jl…

f4fe83c

… files for difference viewing.

deleted intertype versions of amr.jl and decapodes.jl

19f98d2

Removed remaining intertype versions of src and test files for easier…

233896f

… diff view.

p-stokes requested a review from jpfairbanks December 7, 2023 02:43

p-stokes added 2 commits December 7, 2023 10:48

Removed unnecessary imports from test examples.

c2cc82b

Updated Project.toml for docs.

bac39db

jpfairbanks reviewed Dec 7, 2023

View reviewed changes

p-stokes added 3 commits December 7, 2023 14:07

Updated decapodes example doc.

0274d67

Cleaned up imports and qualification of type names in src files. Remo…

37e6398

…ved references to Decapodes.jl in decapodes.jl.

Updated name qualifications in test examples.

bbcb53d

p-stokes requested a review from jpfairbanks December 8, 2023 06:38

jpfairbanks reviewed Dec 8, 2023

View reviewed changes

src/amr.it

element::Vector{Args}

end

#=

Copy link

Member

jpfairbanks Dec 8, 2023

Choose a reason for hiding this comment

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

do we need this?

jpfairbanks reviewed Dec 8, 2023

View reviewed changes

p-stokes added 9 commits December 8, 2023 12:24

Removed new id field from Header. Now same as before the PR. Will mak…

5a9f95b

…e new PR to add later if needed.

Made oapply workaround -- it_to_orig converts an intertype summation …

7785c96

…decapode to a Decapodes one, then Decapodes.Open can be used to compose. Still need to add reverse conversion.

Made coversion from Decapodes.SummationDecapode to intertype version.…

6feef74

… Updated remaining OpenDecapode function to oapply composite model and convert back to intertype.

Uncommented sm_write_json_model, 2nd write_json_model method and most…

ab9a258

… uses in examples. jsonwrite in first method still not working for empty annotations, so those examples are still commented.

Added clarifying comments in composite_models_examples.jl about fixes…

6af6977

… needed to make write_json_model and readback work.

Added acsetspec converter from ADTs version to intertype. Updated typ…

cf77220

…es in .it as needed. Load to AMR from expression examples now work. Need to update amr_to_string (and check other functions) to match current intertypes.

Fixed convert_pair_to_it and changed convert_val_to_it to handle Quot…

403b573

…eNode. Need to update load from dict for ASKEModel and Typing and amr_to_string.

Edited load Typing and ASKEModel from dict. Typing part requires Pair…

dc77e68

…s of Strings for some examples. Also still need to fix amr_to_string and amr_to_expr.

Corrected amr var name in amr_to_expr. Now, problem with amr_to_expr …

e07fb29

…relates to intertype version of ACSetSpec is different format from ADTs. May want to convert back for expr or string. amr_to_string runs but doesn't look good.

olynch closed this Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP update to use InterTypes #29

WIP update to use InterTypes #29

p-stokes commented Dec 1, 2023

codecov bot commented Dec 1, 2023 •

edited

Loading

jpfairbanks left a comment

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

jpfairbanks Dec 6, 2023

p-stokes commented Dec 7, 2023

jpfairbanks Dec 7, 2023

p-stokes Dec 7, 2023 •

edited

Loading

p-stokes Dec 8, 2023

jpfairbanks Dec 7, 2023

p-stokes Dec 7, 2023

p-stokes Dec 8, 2023

jpfairbanks Dec 7, 2023

p-stokes Dec 7, 2023

jpfairbanks Dec 8, 2023

p-stokes Dec 8, 2023

p-stokes Dec 8, 2023

jpfairbanks Dec 7, 2023

p-stokes Dec 8, 2023

p-stokes commented Dec 8, 2023

jpfairbanks commented Dec 8, 2023

jpfairbanks Dec 8, 2023

jpfairbanks Dec 8, 2023

p-stokes commented Dec 8, 2023

olynch commented Jan 17, 2024




		h = AMR.Header("harmonic_oscillator",
		h = Header("","harmonic_oscillator",

WIP update to use InterTypes #29

WIP update to use InterTypes #29

Conversation

p-stokes commented Dec 1, 2023

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

jpfairbanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-stokes commented Dec 7, 2023

Choose a reason for hiding this comment

p-stokes Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-stokes commented Dec 8, 2023

jpfairbanks commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-stokes commented Dec 8, 2023

olynch commented Jan 17, 2024

codecov bot commented Dec 1, 2023 •

edited

Loading

p-stokes Dec 7, 2023 •

edited

Loading