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

WIP: Graph display #53

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

WIP: Graph display #53

wants to merge 5 commits into from

Conversation

rprospero
Copy link
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
Copy link
Member

@trisyoungs trisyoungs left a comment

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!

- *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
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 on lines +23 to +24
- It is difficult for users to create complicated layers without
outside help.
Copy link
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.

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
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.

the modules as individual building blocks.
- One glance at the workflow gives an overview of all of the modules
and their relations

Copy link
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

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
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.

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
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.


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
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 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
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.

`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
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?

### 8.2 Underlying iteration model


Do we truly want an `AbstractListModel` for the `edges` and `nodes`?
Copy link
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