pyiron / pyiron_workflow

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

[minor] Remove `StaticIO.iter` and `.zip` shortcuts #472

Closed liamhuber closed 1 month ago

liamhuber commented 1 month ago

and update quickstart accordingly.

@JNmpi, from #456 it sounds like not only would you like to have different behaviour for node.iter(...), but also that the current behaviour was actively confusing at the hackathon. I would suggest then that we just remove the current behaviour; this avoids confusion immediately, and in the long run frees up .iter and .zip in the node namespace for behaviour that better represents what you want. By removing it now, re-introducing the improved behaviour will avoid breaking backwards compatibility.

review-notebook-app[bot] commented 1 month ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

github-actions[bot] commented 1 month ago

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

codacy-production[bot] commented 1 month ago

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:white_check_mark: -0.09% (target: -1.00%) :white_check_mark:
Coverage variation details | | Coverable lines | Covered lines | Coverage | | ------------- | ------------- | ------------- | ------------- | | Common ancestor commit (f70bac981d607f4179f496cc29f72f4157eb35fb) | 3290 | 3003 | 91.28% | | | Head commit (5ec184f5ad4c7311239e9434f73063681612a56d) | 3278 (-12) | 2989 (-14) | 91.18% (**-0.09%**) | **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 (#472) | 0 | 0 | **∅ (not applicable)** | **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

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

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 11005316414

Details


Files with Coverage Reduction New Missed Lines %
nodes/static_io.py 9 76.32%
<!-- Total: 9 -->
Totals Coverage Status
Change from base Build 10983970779: -0.09%
Covered Lines: 2989
Relevant Lines: 3278

💛 - Coveralls
JNmpi commented 1 month ago

This proposed change goes in the opposite direction of what I wanted to achieve. The main issue is that the current for_loop syntax was considered by many to be too complex and non-intuitive, and there was concern that such constructs would scare people away. The .iter statement, although only briefly discussed, was seen as an easy to use approach. In fact, when the benefits of the workflow approach were highlighted, this was one of the killer features that immediately won people over. So I would really like to keep it, or extend it based on the discussion we had in #456. Rather than removing such a feature, I would like to push the syntax in a way that feels comfortable and intuitive to users. I am fully aware that this is not an easy path, and will require several iterations to arrive at constructs that are easy to use without introducing unwanted or unexpected side effects. But to be successful, this is the path we must take.