"Decouple" strand-tangled components#461
Draft
fedarko wants to merge 27 commits into
Draft
Conversation
lot of caveats, but this works!!! kind of!!! i think the big pain will be adjusting a bunch of code to be like okay, just because we've "drawn a component" does not necessarily mean we've drawn EVERYTHING in this component. Probably we like save the representative / shown / decoupled / whateveryoucallit node and edge IDs/etc somewhere at the Component obj level so that it is easy to eg figure out what paths are available
lots of stuff here, but i think it is moving on well. the big cumbersome thing left looks to be updating various parts of the code to be like "assume that only part of the component might be drawn"
stores edge tuples instead of edge IDs / edge objects (prev commit mixed that up), at least for now. Not sure what format will be best. Using networkx's BFS for traversal seems to work okay -- although I have been thinking about how to handle patterns containing invalidated edges / unshown nodes. Should we try to show such patterns? Or maybe even do pattern decomposition in the decoupled graph? It's not necessary for #449, I think, but might be worth revisiting in the future.
maybe we should instead make _decouple() actually do the logic stuff also
This "just works"! It even mocks the Component number if given so that sorting works as you expect. adjusted the ypad stuff for the hg002 hprc graph to avoid squishing. But I am not sure this way of implementing this is the right move... there are a lot of places which access subgraphs, components, etc, where they assume that like you are using the full component / subgraph / etc. hmmm. It might be safer to just do this the way I was doing it previously, of adjusting the Subgraph / Component .nodes / etc accessors to make them care about scope settings. I think that would be less error-prone, since then we'll need to like actually go through and update the places that rely on this.
not sure if we will be sticking with this, but the more i think abt it the better it sounds
& move from Subgraph. I think this makes sense since this doesnt really have meaning for non-Component Subgraphs
yay! this is the core functionality we want to have. There is a laundry list of things that I want to do before merging this in. Summarizing what I wrote down on paper today: - clean up code - test code - There can be weird cases where the nodes of an inval edge happen to be in the same pattern, even after adjusting the src or tgt of the inval edge based on the shown nodes. An example of this is sample_LastGraph -- see nodes 43 and 35. Caused by two adjacent RC frayed ropes. We should test (just messing around in GV) to see if putting these edges in the pattern cluster makes the layout look nicer. If so, might be worth detecting these cases and doing that in practice. - Decoupling will not work properly with recursive layout, and with drawing "around" nodes. It could be adapted to both of these given enough time and effort (particularly drawing around nodes), but for now I think it would be best to just *disable* decoupling if either of these other options is selected. (For now, I've created #460 and #459 for these.) - Searching for X should show -X if only -X is drawn (#407) - Detect unnecessary split nodes relative to the decoupled graph, and merge them (#264) - Use tooltips to explain what this stuff is even doing (#29) - maaaaybe put edge ports on by default, etc (#454) - I am not sure if parallel loop edges induced by decoupling are getting treated properly -- see 249210759 in chr15_full test graph. Shouldn't there be two of these edges? Need to look into it.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
==========================================
- Coverage 71.02% 70.82% -0.21%
==========================================
Files 34 35 +1
Lines 4394 4589 +195
Branches 1073 1119 +46
==========================================
+ Hits 3121 3250 +129
- Misses 1180 1232 +52
- Partials 93 107 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
For now, it's shown as bolded in the Graphviz layout (just for debugging atm, but will be relevant to #429); and it's shown in the Cy.js viz (ie the main thing) as a dashed line with much larger "blocks" than fake edges. I like the larger-block-dashed-line way of encoding this because - we don't have to mess around with colors, and - we don't have to mess around with line widths but we can ofc add settings later on to make these super obvious and garish if desired ...
Fixes a bug -- if there were ordinary edge(s) from A to B and also
invalidated edges from A to B (at least, like, in terms of the layout
-- really such edges were like actually from -A to B or A to -B) then
the old code didn't notice which was which in the layout.
Basically this meant that the fancy port-changed stuff could have
been assigned to the non-invalidated edge, and vice versa. oh no!
Test case is 57-R --> 4 (and 57-R --> -4) in sample_LastGraph.
This fixes things by storing key={edge.unique_id} in the DOT we
create for PyGraphviz. I guess the big wonderful thing here is that
the key attributes we chuck into the DOT string are interpreted
correctly by PyGraphviz, and can be used later on to pull out their
corresponding edges' layout info.
Oof, would it be worth trying to figure out how far back this
functionality in pygraphviz is supported ...? Fortunately it seems
like if you pass key=xyz when trying to get an edge from PyGraphviz
then it will just crash, so that's fine by me - just be loud.
also restructured some of the layout utils code while i'm at it.
a lot of silly function orderings (so many levels to parsing
control point stuff!)
i think this is good but gotta think more abt
Basically just marking the stuff that isn't used outside of this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #449.
HG002 HPRC graph, using decoupling
HG002 HPRC graph, no decoupling