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

mnt: nodes context settings are no longer a regular input #547

Merged
merged 2 commits into from
Mar 8, 2023

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented Mar 8, 2023

Closes #538

Previously, settings that controlled the behavior of nodes where passed as function arguments, which was confusing:

from sisl.nodes import Node

@Node.from_func
def sum_node(a, b):
    return a + b

my_node = sum_node(1, 2, automatic_recalc=True)

Now it is instead controlled with contexts. A node's behavior is defined by its context, which is a chainmap from sisl's main context (sisl.nodes.context.MAIN_CONTEXT), the node class context (Node.context), the instance context and a global temporal context.

Therefore, the same behavior can be obtained in different ways, which act at different levels:

from sisl.nodes import Node, temporal_context

@Node.from_func
def sum_node(a, b):
    return a + b

my_node = sum_node(1, 2)

# Option 1: Modify the global sisl context, which affects all nodes
sisl.nodes.MAIN_CONTEXT.update(lazy=False)

# Option 2: Set the laziness for all instances of a Node
Node.context.update(lazy=False)

# Option 3: Set the laziness for all instances of a sum_node
sum_node.context.update(lazy=False)

# Option 4: Set the laziness only for this instance
my_node.context.update(lazy=False)

# Option 5: Set a temporal context, which overrides all contexts.
with temporal_context(lazy=False):
    ... do whatever

I also changed what lazy means. Before this PR it defined what was returned when calling a node class. If lazy, a node instance was returned and if not, the result was directly returned. Since it is now very easy to go from function to node and viceversa I thought that calling a node class should always return a node instance to avoid confusion. Now lazy means just whether the node should be recomputed on change of inputs, which was the old automatic_recalc, now removed.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #547 (6667362) into main (d7fd5af) will decrease coverage by 0.02%.
The diff coverage is 1.08%.

❗ Current head 6667362 differs from pull request most recent head 66576b1. Consider uploading reports for the commit 66576b1 to get more accurate results

@@            Coverage Diff             @@
##             main     #547      +/-   ##
==========================================
- Coverage   86.25%   86.24%   -0.02%     
==========================================
  Files         366      367       +1     
  Lines       46500    46465      -35     
==========================================
- Hits        40107    40072      -35     
  Misses       6393     6393              
Impacted Files Coverage Δ
sisl/nodes/__init__.py 0.00% <0.00%> (ø)
sisl/nodes/context.py 0.00% <0.00%> (ø)
sisl/nodes/node.py 0.00% <0.00%> (ø)
sisl/nodes/workflow.py 0.00% <0.00%> (ø)
sisl/geometry.py 86.09% <100.00%> (-0.36%) ⬇️
sisl/io/sile.py 86.32% <0.00%> (-0.40%) ⬇️
sisl/io/xyz.py 97.10% <0.00%> (-0.16%) ⬇️
sisl/io/tests/test_xyz.py 100.00% <0.00%> (ø)
sisl/io/siesta/__init__.py 100.00% <0.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sisl/nodes/context.py Outdated Show resolved Hide resolved
@zerothi
Copy link
Owner

zerothi commented Mar 8, 2023

Great!!!

I'll just merge!

@zerothi zerothi merged commit e4149e4 into zerothi:main Mar 8, 2023
@pfebrer pfebrer deleted the nodes_bug branch March 9, 2023 09:23
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.

sisl.nodes and automatic_recalc
2 participants