Redesign of Julia interop, avoiding Jupyter kernel #835
Conversation
epatters
force-pushed
the
update-cclinterop-types
branch
from
967ea9f to
113a4f8
Compare
kris-brown
force-pushed
the
interop_refactor
branch
from
b9eb474 to
4d6e69e
Compare
kris-brown
force-pushed
the
interop_refactor
branch
from
4d6e69e to
3b4ce9b
Compare
CLEANUP: changing tests CLEANUP: reorganized code into parse and execute folders refactor algjuliainterop CORS and async issues resolved basic tabular view functionality edit remove decapodes material run formatter docs fix using local version remove axios undo changes newlines remove space More docs
kris-brown
force-pushed
the
interop_refactor
branch
from
34dadc8 to
5d8dd71
Compare
kasbah
left a comment
kasbah
left a comment
There was a problem hiding this comment.
Some more CORS fixups for you.
|
Thanks, these are addressed now! |
epatters
left a comment
epatters
left a comment
There was a problem hiding this comment.
Thanks Kris and Matt, it'll be great to have a simpler and more flexible approach to interop with Julia. Some comments below.
| const model = props.liveDiagram.liveModel.elaboratedModel(); | ||
| if (model === undefined) { | ||
| throw "Bad model"; | ||
| } |
There was a problem hiding this comment.
You don't want to throw an error here and this code looks redundant anyway since you're already extracting the elaborated model in the Solid resource. I think you can just delete the above lines:
const model = props.liveDiagram.liveModel.elaboratedModel();
if (model === undefined) {
throw "Bad model";
}|
|
||
| export const tabularView = ( | ||
| options: AnalysisOptions, | ||
| ): DiagramAnalysisMeta<GraphLayoutConfig.Config> => ({ |
There was a problem hiding this comment.
Using the graph layout config doesn't make sense. Presumably a holdover from copy-pate.
If I'm not mistaken, this analysis has no content/state of its own, so you can replace
DiagramAnalysisMeta<GraphLayoutConfig.Config>with
DiagramAnalysisMeta<{}>| ): DiagramAnalysisMeta<GraphLayoutConfig.Config> => ({ | ||
| ...options, | ||
| component: (props) => <TabularView title={options.name} {...props} />, | ||
| initialContent: GraphLayoutConfig.defaultConfig, |
There was a problem hiding this comment.
This then becomes
initialContent: {},| } | ||
|
|
||
| /** Given a schema (DblModel of ThSchema), a JSON output `rawdata` from Catlab, | ||
| and a particular object `obname` in the schema, create an HTML table with |
There was a problem hiding this comment.
Docstring seems to end mid-sentence.
| }, | ||
|
|
||
| async ([model, diagram]) => { | ||
| const response = await fetch("http://127.0.0.1:8080/acsetcolim", { |
There was a problem hiding this comment.
Since this feature currently requires a server running locally, I think we should treat it as "experimental" and thus not show this analysis in prod. I'll suggest elsewhere how to do that.
| ```sh | ||
| julia --project -e 'import Pkg; Pkg.instantiate()' | ||
| ``` | ||
| [AlgebraicJulia](https://www.algebraicjulia.org/) available to CatColab.At this |
There was a problem hiding this comment.
Typo: missing space after "CatColab."
| (m) => | ||
| obname === | ||
| model | ||
| .obGeneratorLabel(model.morPresentation(m)?.dom.content.toString() || "") |
There was a problem hiding this comment.
As another rule of thumb, you should never have to call toString() when working with presentations. Calling it suggests that you haven't narrowed the type enough. I'd rewrite the above predicate as
(id) => {
const mor = model.morPresentation(id);
return mor.dom.type === "Basic" && obname === model.obGeneratorLabel(mor.dom.content);
}(But that's assuming that you're still keying by object labels, which I wouldn't recommend. See my other comment.)
| /** Given a schema (DblModel of ThSchema), a JSON output `rawdata` from Catlab, | ||
| and a particular object `obname` in the schema, create an HTML table with | ||
| */ | ||
| function createACSetTable(model: DblModel, rawdata: object, obname: string) { |
There was a problem hiding this comment.
It's a code smell that rawdata has type object. You should have a type def of the format of the data expected to be returned from CatColab.
| */ | ||
| function createACSetTable(model: DblModel, rawdata: object, obname: string) { | ||
| // The primary key of this table is given by `rawdata[obname]` | ||
| const rows: Array<string> = rawdata[obname as keyof typeof rawdata]; |
There was a problem hiding this comment.
As a rule of thumb, when working with models, you should always key mappings by ID rather than human-readable label, which (1) are not guaranteed to be unique and (2) are structured objects (currently, lists of strings, rather than strings).
I would suggest that the last argument to this function be obId: string rather than obname and then
const rows: Map<string, string> = new Map();Note that Maps in JavaScript (unlike JSON objects) are guaranteed to preserve the insertion order, so you don't need to worry about losing ordering.
|
|
||
| // Convert morgenerators to user-friendly names | ||
| const headers = [obname].concat( | ||
| outhoms.map((m) => model.morGeneratorLabel(m)?.toString() || ""), |
There was a problem hiding this comment.
We should probably have a helper function for this, but the correct way to get a human-friendly name is
model.morGeneratorLabel(m)?.join(".") ?? ""
This PR simplifies a lot of the structure of the CatColabInterop.jl package.
It introduces a new analysis of schema instances by sending the diagram to Catlab, where it is processed by
@acset_colimto produce a tabular instance.(This is currently a WIP - once the workflow is working for the tabular view analysis, the existing Decapodes analysis will be brought back into functionality).