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

[patch] change color when failed #462

Merged
merged 2 commits into from
Sep 20, 2024
Merged

[patch] change color when failed #462

merged 2 commits into from
Sep 20, 2024

Conversation

samwaseda
Copy link
Member

@samwaseda samwaseda commented Sep 19, 2024

Closes #92

from pyiron_workflow import Workflow, function_node

def add(x, y):
    output = x + y
    return output

def mul(x, y=2):
    raise ValueError
    output = x * y
    return output

wf = Workflow("draw")
wf.Add = function_node(add, x=1, y=2)
wf.Mul = function_node(mul, x=wf.Add)

wf.run()
Screenshot 2024-09-20 at 08 25 22

Btw. does it have a particular reason that node has a color?

Copy link

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_workflow/change_color_for_failed

@samwaseda samwaseda added the enhancement New feature or request label Sep 19, 2024
Copy link

codacy-production bot commented Sep 19, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.00% (target: -1.00%) 92.86%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (89ddd30) 3243 2960 91.27%
Head commit (a07ef28) 3256 (+13) 2972 (+12) 91.28% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#462) 14 13 92.86%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@coveralls
Copy link

coveralls commented Sep 19, 2024

Pull Request Test Coverage Report for Build 10954304912

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 13 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 91.278%

Files with Coverage Reduction New Missed Lines %
draw.py 13 92.56%
Totals Coverage Status
Change from base Build 10943294792: 0.004%
Covered Lines: 2972
Relevant Lines: 3256

💛 - Coveralls

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

I really liked how for the present/missing data you did just the outline -- could we port that to doing the outline in red here and leaving the fill the usual colour?

@liamhuber
Copy link
Member

Btw. does it have a particular reason that node has a color?

The end result should be that nodes of different types should have different colours (macros, function nodes, etc.) to improve visual clarity.

From a purity perspective, this should absolutely not be a property of the node -- the draw method (or pyiron-xyflow) should handle it. But from a practical implementation perspective, that would mean that the drawer has its own colour map and is constantly doing isinstance checks to see which colour should be used -- grabbing node.color is much faster. I'm totally ok either route -- pure or practical, but practical is what there is right now.

@samwaseda
Copy link
Member Author

From a purity perspective, this should absolutely not be a property of the node -- the draw method (or pyiron-xyflow) should handle it. But from a practical implementation perspective, that would mean that the drawer has its own colour map and is constantly doing isinstance checks to see which colour should be used -- grabbing node.color is much faster. I'm totally ok either route -- pure or practical, but practical is what there is right now.

ok it also sounds to me like we should have a possibility to export levels, and it sounds to me like it's something that pyiron_workflow should be able to do. I actually kinda have the feeling that the drawing module should also be better separated. I'll look into it at some point.

@samwaseda
Copy link
Member Author

I really liked how for the present/missing data you did just the outline -- could we port that to doing the outline in red here and leaving the fill the usual colour?

I updated it and also added an example in the top comment.

@liamhuber liamhuber self-requested a review September 20, 2024 14:33
@liamhuber
Copy link
Member

we should have a possibility to export levels

Can you clarify this? IMO it's not as clear as "levels" of node -- that makes sense for function vs macro, but not so much for function vs transformer, or for loop vs macro.

@liamhuber
Copy link
Member

I actually kinda have the feeling that the drawing module should also be better separated.

I see what you're saying, but I will disagree; I think it's nice to have some sort of built-in way of visualizing the nodes that comes with the library. It's a common enough operation that I'm disinclined to force people to separately install a second tool to get it done.

@liamhuber liamhuber changed the title change color when failed [patch] change color when failed Sep 20, 2024
@liamhuber liamhuber merged commit 83a7f68 into main Sep 20, 2024
17 checks passed
@liamhuber liamhuber deleted the change_color_for_failed branch September 20, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodes in a failed state should be glaringly obvious in the graph draw
3 participants