Skip to content

WIP: Graph display#53

Open
rprospero wants to merge 5 commits into
mainfrom
GraphDisplay
Open

WIP: Graph display#53
rprospero wants to merge 5 commits into
mainfrom
GraphDisplay

Conversation

@rprospero

Copy link
Copy Markdown
Contributor

This is a rough work in progress on a description of the graph library. I'm only pushing it at this point to confirm that the diagrams render correctly.

@rprospero rprospero marked this pull request as ready for review September 6, 2024 14:35

@trisyoungs trisyoungs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't mind the RFC template as it stands, although some of the language (e.g. "Dashboards") is too project-y for my taste!

Comment thread RFC/0000-template.md
- *including code examples if you're proposing an interface or system contract.*
- *linking to project briefs or wireframes that are relevant.*

## 4 Metrics & Dashboards

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Dashboards"? I think measurable metrics is fine, and all we need to capture the product success / scope.

Comment thread RFC/0001-graph-display.md
Comment on lines +23 to +24
- It is difficult for users to create complicated layers without
outside help.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- It is difficult for users to create complicated layers without
outside help.
- It is impossible to create complex workflows in the current linear layout, hindering the development and use of Dissolve for advanced scientific analysis.

Comment thread RFC/0001-graph-display.md
activities of a layer. Each module much be read in sequence and a
mental modal of the entire, possibly non-linear, process must be
constructed from this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- Usage relationships between data / objects are "weak", leading to significant data-management within the code in order to maintain a valid state.

Comment thread RFC/0001-graph-display.md
the modules as individual building blocks.
- One glance at the workflow gives an overview of all of the modules
and their relations

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Links between data / objects is well-defined and strong, removing data management issues almost completely

Comment thread RFC/0001-graph-display.md
signals from the interface. The `createNode` slot takes a string that
corresponds to the `type` property of the Node Interface and appends a
new Node of the correct type. Correspondingly, `deleteNode` takes the
index of a node and removes that node from the model. In the same

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be wise to continue demanding unique names for nodes / modules, so we don't need to access by index and could use the object name instead.

Comment thread RFC/0001-graph-display.md
This could have a significant impact on the structure of the input
file, since it represents a major change in the state of the data.
Furthermore, this may be a significant break change, as it would not
be a trivial matter to convert existing, linear code into the new

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This work would really represent a huge change - Dissolve 2 - and one which would break basically every existing input file. It might be possible to write some kind of importer for 1.X TOML input, but it may be difficult and perhaps not worth the effort. I would be happy to accept the fact that Dissolve 2 is a new starting point for everyone's simulations. One might argue there is little value to the user in importing an old simulation, since they will need to get used to the new workflows we'll be presenting.

Comment thread RFC/0001-graph-display.md

This first method is to simply forbid invalid graphs. The model
should reject any attempt to create invalid edges or delete nodes that
are required. This had advantages in design and implementation, but

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
are required. This had advantages in design and implementation, but
are required. This has advantages in design and implementation, but

Comment thread RFC/0001-graph-display.md
Comment on lines +199 to +201
places limits on what can provide a good user experience. For
example, every possible node input must have a **valid** default
value, so that the connection to that input can be deleted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that strictly disallowing connections between dissimilar input / output types is the best route to go - something like Blender will allow you to connect related type inputs/outputs and still yield sensible results, but I don't think we need that softness here. Regarding the need for default values, this is what we currently have with keywords in modules and nodes, so not a problem moving forward.

Comment thread RFC/0001-graph-display.md
`AbstractItemModel`. I chose `AbstractListModel` since it is the
easier to subclass, but all of these models have an element of
ordering that we currently do not care about. If we do care in the
future, it might be better to have used `AbstractListModel`, but that

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess you meant either AbstractTableModel or AbstractItemModel here?

Comment thread RFC/0001-graph-display.md
### 8.2 Underlying iteration model


Do we truly want an `AbstractListModel` for the `edges` and `nodes`?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably we do, at least based on how I'm understanding your proposal here as it stands. Creating this as a light framework feels like the best option, with the specific functionality we need only.

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.

2 participants