-
Notifications
You must be signed in to change notification settings - Fork 24
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
Merging nodes with same name but different interfaces drops further nodes without telling #14
Comments
Diagrams above taken from a new TC in https://github.com/ankostis/graphkit/tree/fix14, assembled from the gist in the OP. |
Wait, so the merging is actually producing 2 graphs? I think I missed that one... how do you collect the second one? I can't find any difference between your |
Even if both graphs are somehow gathered, I still see a problem: If the merging operation is not able to keep this dependency, I still think it should terminate with an error or at least notify the user. |
Updated diagrams, adding the inverse merge ( @andres-fr I notice that the description in the OP does not accurately depict hte situation in the diagrams. Would you care to update it? |
@ankostis sure, but for that I would need to know how are you able to extract 2 graphs after merging. How are you collecting the second one? In my code I just get the "bigger" one and for that case the OP would describe the situation... |
I'm running py3.7, and maybe ordered dicts have something to do with it. [edit] |
Yeah, I'm sorry but I will probably switch to It gets some more effort to get into it, but then you can be sure that it just works. Without extending too much, I also love the possibility of creating singleton, factory and config types out of the box. In any case, it is a different concept and not necessarily a competitor... I want to thank you very much for your very informative table and your great help with this project. If no further people jump in I am closing this issue. Cheers! |
NOTE dict are not deterministic in <PY3.6. So this commit would not improve determinism in those pythons. + build: add `boltons` dependency for ndexedSet. + doc: mark all set usage if affect determinism. + e.g. see reproducibility problem in yahoo#14.
Hi yahooers! Thanks for this very helpful software. I noticed one issue at graph merging, seems like a bug. I have version 1.2.4
Scenario:
graph1 = abs(a + (a * b))**3
, with operations named(mul1, sum1, abspow1)
graph2 = abs(a + (a * b))**2
, with operations named(mul1, sum1, abspow2)
graphkit.compose(name="graph3", merge=True)(graph1, graph2)
sum1
operations have same name, but crucially different output namesabspow1
depends onsum1
, andabspow2
depends onsum2
In this scenario, two options would be acceptable when evaluating the merged graph:
mul1
(same name AND interface) and leave the rest as independent branchessum2
intosum1
, and makeabspow2 point to
sum1`But what is happening is that both the second
sum1
andabspow2
are being dropped, without any warning or exception thrown. Also, the squared output ofabspow2
is missing from the merged graph's output.I assume the reason is that the merged graph can't fullfil
abspow2
's interface aftersum1
's "faulty" merge and is dropping it silently... Is this behaviour expected? The docs are pretty clear about the "merging by name process", but I feel like this scenario could be problematic if a user inadvertently messes up the interfaces, and whole branches of the graph are dropped.In case this is unexpected, these could be 2 possible approaches:
InterfaceConfictError
and dismissing the merge (more conservative)sum2
point tosum1
after they merge, regardless of their interface (would require some assumptions on the interface or potentially a redesign of the whole library into a statically typed, TF-ish ̶n̶i̶g̶h̶t̶m̶a̶r̶e̶ experience)I uploaded a
unittest
with a reproducible example into this gist, It is in thetest_graph_merge_dropping
method. Let me know if I can be of any help via PR.Cheers,
Andres
The text was updated successfully, but these errors were encountered: