pyiron / pyiron_workflow

Graph-and-node based workflows
BSD 3-Clause "New" or "Revised" License
10 stars 1 forks source link

Add error message for return item without `channel` #241

Closed samwaseda closed 3 months ago

samwaseda commented 3 months ago

I keep making a mistake like this:

@single_value_node("sum")
def addition(a, b):
    return a + b

@single_value_node("product")
def multiplication(a, b):
    return a * b

@macro_node("result")
def add_and_multiply(macro):
    macro.addition = addition(1, 2)
    macro.multiplication = multiplication(macro.addition, 5)
    return macro.multiplication.outputs.product.value

node = add_and_multiply()

The main problem is that in the current implementation it only says NOT_DATA does not have channel, which doesn't immediately make me understand where the error is coming from.

github-actions[bot] commented 3 months ago

Binder :point_left: Launch a binder notebook on branch _pyiron/pyiron_workflow/macro_add_returnerror

codacy-production[bot] commented 3 months ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:white_check_mark: -0.01% (target: -1.00%) :white_check_mark: 83.33%
Coverage variation details | | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (bb326ec6d26efd371eac45d431c0d3d6d1dc680a) | 3146 | 2730 | 86.78% | | | Head commit (27fbb46a77a470f576447c2a6c48dea580bc405f) | 3151 (+5) | 2734 (+4) | 86.77% (**-0.01%**) | **Coverage variation** is the difference between the coverage for the head and common ancestor commits of the pull request branch: ` - `
Diff coverage details | | Coverable lines | Covered lines | Diff coverage | | ------------- | ------------- | ------------- | ------------- | | Pull request (#241) | 6 | 5 | **83.33%** | **Diff coverage** is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: `/ * 100%`

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 8254926325

Details


Files with Coverage Reduction New Missed Lines %
macro.py 12 89.4%
<!-- Total: 12 -->
Totals Coverage Status
Change from base Build 8210165337: -0.01%
Covered Lines: 5841
Relevant Lines: 6363

💛 - Coveralls
liamhuber commented 3 months ago

Oh, and we need a test added that catches the new message

liamhuber commented 3 months ago

Ok, I've got my computer and I stand by everything I said earlier. More specifically, somewhere in here:

https://github.com/pyiron/pyiron_workflow/blob/bb326ec6d26efd371eac45d431c0d3d6d1dc680a/pyiron_workflow/macro.py#L313-L319

I'd slot in a new method like self._ensure_returned_objects_are_has_channels (or similar) that does the check you want. This is perfectly in line with the variable name, it's just that right now the code assumes that the user has correctly returned only HasChannel objects. I totally agree that's an easy mistake though, so let's add the filtering. To fail as early as possible I recommend inserting the check immediately after line 313.

In the future we could even imagine extending the check to create channel objects from non-HasChannel returns. This heads in the direction of getting macro and function nodes to be more similar, and is absolutely not necessary here and now, but I like that this change is commensurate with that direction.

samwaseda commented 3 months ago

Ok I messed around. In the end I didn't put it in __init__, because for my taste there are already too many lines. I also added a test.

liamhuber commented 3 months ago

Ugh, my rerun failed jobs button has no effect. I'm not sure if it's a problem with my browser or what, but this happens sometimes. @samwaseda could you rerun the failed windows test before merging?