github-actions[bot]
commented
1 month ago 
:point_left: Launch a binder notebook on branch _pyiron/pyiron_workflow/guicolors
Open Tara-Lakshmipathy opened 1 month ago
github-actions[bot]
commented
1 month ago :point_left: Launch a binder notebook on branch _pyiron/pyiron_workflow/guicolors
codacy-production[bot]
commented
1 month ago | Coverage variation | Diff coverage |
|---|---|
| :white_check_mark: -0.15% (target: -1.00%) | :white_check_mark: 50.00% |
Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more
coveralls
commented
1 month ago | Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| nodes/function.py | 3 | 95.45% | ||
| nodes/composite.py | 3 | 91.71% | ||
| nodes/transform.py | 4 | 97.13% | ||
| <!-- | Total: | 10 | --> |
| Totals | |
|---|---|
| Change from base Build 11097297857: | -0.1% |
| Covered Lines: | 3094 |
| Relevant Lines: | 3383 |
Tara-Lakshmipathy
commented
1 month ago The properties are used in pyiron_xyflow here to color the nodes.
samwaseda
commented
1 month ago can you maybe also say real quick what was the problem without having gui_color?
Tara-Lakshmipathy
commented
1 month ago The colors in the gui are taken directly from the nodes in pyiron_workflow which are themselves defined in colors.py in pyiron_snippets. These colors are too dark for the gui.
samwaseda
commented
1 month ago Ok that's a valid concern. But I have a different concern now: Since we rely on auto-complete as well as __setattr__ and __getattr__, it might be preferable to keep the number of attributes as low as possible. Maybe the easiest solution for now is to change color by get_color and take an argument for GUI. However, I have the feeling that the real source of the problem is the fact that the node objects (nodes, macros etc.) have colors. Maybe it's really time to think about changing this.
Let's wait for @liamhuber and see what he says
Tara-Lakshmipathy
commented
1 month ago Ok that's a valid concern. But I have a different concern now: Since we rely on auto-complete as well as
__setattr__and__getattr__, it might be preferable to keep the number of attributes as low as possible. Maybe the easiest solution for now is to changecolorbyget_colorand take an argument for GUI. However, I have the feeling that the real source of the problem is the fact that the node objects (nodes, macros etc.) have colors. Maybe it's really time to think about changing this.Let's wait for @liamhuber and see what he says
There may be more such attributes for the automated positioning of nodes in the gui in the future. If not in the specific types of nodes (e.g., function.py) then at least in node.py. So, maybe it's already worth discussing where such attributes should go.
liamhuber
commented
1 month ago I am definitely against introducing a new field for the gui color -- the core infrastructure shout have no idea the gui exists, and @samwaseda is correct about tab completion menu crowding.
I have no objection to updating the existing color attributes and that should solve the problem.
I am ok totally getting rid of the color node attribute, but this information can't be destroyed -- only moved. Somewhere we should have a table relating classes to colors, and a function for getting most-specific class color from this table given the class. Pros: nodes don't need to know about colors and then they wouldn't, and draw and the guy could use the same infrastructure with different color tables. Cons: more of a pain to maintain as such color tables may need to be updated (maybe in multiple places) if a new node is introduced. Overall I still think this is the best attack.
samwaseda
commented
1 month ago the core infrastructure shout have no idea the gui exists
And for this very reason I find it actually surprising that there are colours attached to the nodes in the first place. For me a helpful distinction would be whether there are more nodes underneath, which in the case of pyiron_workflow coincides with node vs. macro, but I guess we can also do the same distinction by whether it has children or not?
One way or other, I think there should be a cleaner interface between node and draw. Ideally pyiron_workflow should be able to export an entire graph first in a simpler format like dict, which is taken over by draw.py. And from my point of view, it should be entirely up to draw.py to figure out how to plot it.
For this PR, if it's all about darkness, we can maybe define something similar to lighten_hex_color for now on the GUI side?
Tara-Lakshmipathy
commented
1 month ago Is there an attribute which gives me the type of the node - function, composite etc.? I feel this would be useful in a variety of situations. On the S-Bahn for the next 50 minutes, but can't wait that long to check for myself 😆.
EDIT: nvm I saw in the draw module that isinstance() is used. But I think having an attribute that returns the type of node directly would be a neat convenience feature for a bunch of front-end applications, especially when an external tool wants to interface with 'pyiron_workflow' without knowing about the internal python classes used in the workflow code. Also convenient for filling metadata fields on publishing platforms.
liamhuber
commented
1 month ago And for this very reason I find it actually surprising that there are colours attached to the nodes in the first place.
One way or other, I think there should be a cleaner interface between
nodeanddraw
Yes, this is absolutely just bad/lazy abstraction on my part. This information needs to reside inside the pyiron_workflow package, because it's important to me that the base package ships with the ability to provide a non-code representation (i.e. draw), but the colour data and logic absolutely should all be inside the draw module.
For me a helpful distinction would be whether there are more nodes underneath, which in the case of pyiron_workflow coincides with node vs. macro, but I guess we can also do the same distinction by whether it has children or not?
Is there an attribute which gives me the type of the node - function, composite etc.?
What you're looking for is just the class inheritance -- you just want isinstance here and a prioritized dict of type[Node]: str to match classes to colors.
I agree the main two are Function and Macro. Beyond this, I also think it's useful for Transformer (the parent class for dataclass nodes, list-to-output, input-to-list, etc), For, and If all to get unique colors. I'm not clairvoyant, but -- unless we introduce a specific While node instead of just constructing it as a macro -- that set should provide colors for the foreseeable future.
Ideally pyiron_workflow should be able to export an entire graph first in a simpler format like
dict
This exists for Composite nodes (macros and workflows) -- Composite.graph_as_dict! draw was one of the first things I implemented, and graph_as_dict was a convenience method added much later (you can go look, it is really simply iterating over things the nodes already provide easy access to), thus in the spirit of "if it's working, don't touch it", I never tried to leverage the new convenience in draw. In the spirit of "refactor, refactor, refactor", if you want to leverage this in draw, or find a convenient way to pull the method from Composite up to Node and use that, I'm on board.
liamhuber
commented
1 month ago But I think having an attribute that returns the type of node directly...
I disagree -- there is a straightforward pythonic way of finding what type of node we have and it is type(node) or isinstance(node, ThingIAmCheckingAgainst). This is already super straightforward and I don't think we should reinvent the wheel/create more
...would be a neat convenience feature for a bunch of front-end applications, especially when an external tool wants to interface with 'pyiron_workflow' without knowing about the internal python classes used in the workflow code.
Here I agree, but the way I suggest to do it is as I mentioned earlier: pyiron_workflow.draw, which is embedded right in the module and knows all the core classes, should know how to map those core classes to nice default colours. This is convenient without stepping on python's toes, and should cover 90%+ of use cases. If someone wants a non-standard colour for some particular node, nothing stops them from doing that, and whatever pyiron_workflow.node.get_color(node: Node) looks like, it should be flexible enough that they can extend it. E.g. perhaps it's like pyiron_workflow.draw.get_color(node: Node, color_map: Optional[dict]: None) and we can also provide pyiron_workflow.draw.color_map: dict[type[Node], str] that they could import, extend/override, then pass to get_color.
So I'm for convenience, but against alternate routes for standard language stuff.
Tara-Lakshmipathy
commented
1 month ago If we have get_color as opposed to the color we have now, then I would be in agreement. And since this pull request is about colors, I don't want to deviate from that topic too much. But I agree with @samwaseda mentioned earlier. I was surprised to find color in these files. Initially I expected to find it in draw.py as well. So if we all agree to attack this by getting rid colors from the nodes themselves, then count me in.
codacy-production[bot]
commented
1 month ago | Coverage variation | Diff coverage |
|---|---|
| :white_check_mark: -0.15% (target: -1.00%) | :white_check_mark: 50.00% |
Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more