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

Fix delete_node to allow breaking cycles in graphs. #75

Merged
merged 6 commits into from
Apr 4, 2023

Conversation

gkirgizov
Copy link
Collaborator

Previously they were automatically preserved because parents were always reconnected to children.

This change is backward compatible except for 1 rename (RemoveType.node_only -> RemoveType.node_rewire), no current behavior is altered.

Related to #69 where cycles that appeared in graph precluded optimization from removing accidental cycles.

@gkirgizov gkirgizov added the enhancement New feature or request label Mar 29, 2023
@MorrisNein MorrisNein self-requested a review March 29, 2023 07:33
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #75 (cee630d) into main (75da6f6) will decrease coverage by 0.47%.
The diff coverage is 27.27%.

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
- Coverage   65.59%   65.12%   -0.47%     
==========================================
  Files         113      114       +1     
  Lines        6162     6220      +58     
==========================================
+ Hits         4042     4051       +9     
- Misses       2120     2169      +49     
Impacted Files Coverage Δ
golem/core/adapter/nx_adapter.py 97.72% <ø> (+24.39%) ⬆️
golem/core/optimisers/objective/objective.py 86.27% <ø> (ø)
golem/metrics/edit_distance.py 0.00% <0.00%> (ø)
golem/metrics/graph_metrics.py 0.00% <0.00%> (ø)
...ore/optimisers/genetic/operators/base_mutations.py 89.01% <40.00%> (-0.55%) ⬇️
golem/core/dag/graph.py 78.82% <100.00%> (+1.32%) ⬆️
golem/core/dag/graph_delegate.py 96.07% <100.00%> (ø)
golem/core/dag/linked_graph.py 100.00% <100.00%> (ø)
golem/core/optimisers/advisor.py 88.88% <100.00%> (+0.65%) ⬆️
golem/core/optimisers/genetic/gp_operators.py 92.85% <100.00%> (-0.17%) ⬇️

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

Copy link
Collaborator

@maypink maypink left a comment

Choose a reason for hiding this comment

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

я бы еще параметр clean_up_leftovers у метода disconnect_nodes по дефолту убрала в False

golem/core/dag/graph.py Outdated Show resolved Hide resolved
test/unit/dag/test_graph.py Show resolved Hide resolved
golem/core/dag/linked_graph.py Outdated Show resolved Hide resolved
@MorrisNein
Copy link
Collaborator

MorrisNein commented Mar 31, 2023

RemoveType.with_direct_children, как я понял, нигде не используется и не тестируется. Так что я не уверен, насколько это важное наблюдение

Но кажется, что дефолтное поведение delete_node(..., reconnect=ReconnectKind.single) может сломать вот это место:

https://github.com/aimclub/GOLEM/pull/75/files#diff-22597fa80790447c12350557295b46256d87b8a700fcc1b6c0331f6b49765141L241-L249

Представим граф:

1 \
2 -> 4 -> 5 -> 6
3 /

Допустим, мы пытаемся удалить узел 4 с прямыми потомками. Пошагово:

  1. Удаляем узел 4, не происходит реконнект:
1 \
2 -> x -> 5 -> 6
3 /
  1. Удаляем дочерний узел 5:
1 \
2 -> х -> 6
3 /

Реконнекта так и не произошло, случилась кака

Возможно, стоит в этом месте использовать delete_node(..., reconnect=ReconnectKind.all)

Этот момент легко проморгать в будущем, если станем использовать эту опцию

@gkirgizov
Copy link
Collaborator Author

Возможно, стоит в этом месте использовать delete_node(..., reconnect=ReconnectKind.all)

Этот момент легко проморгать в будущем, если станем использовать эту опцию

Поправил, хорошо что заметил, тонкий момент!

@gkirgizov gkirgizov requested a review from maypink April 3, 2023 06:04
@gkirgizov gkirgizov merged commit b5b9878 into main Apr 4, 2023
@gkirgizov gkirgizov deleted the fix-drop-cycle-mutation branch April 4, 2023 03:33
@gkirgizov gkirgizov mentioned this pull request Apr 4, 2023
1 task
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.

4 participants