feat: drop the use of node attribute "first_nbr" in PlanarEmbedding #7202
Conversation
…e _cw and _ccw varieties, plus update the planar_drawing and tests accordingly (planar_drawing.py, planarity.py, test_planarity.py)
There was a problem hiding this comment.
This seems like a good performance choice -- and it simplifies the methods api so probably better for reading the code too. Thanks!
Could you test the new function to ensure that it raises when it should, e.g. cw and ccw specified and the other cases? There is a nice match option on pytest.raises so you can put in a key part of the error message to make sure the right one gets caught.
It looks like the other (existing) tests put building the PlanarEmbedding inside the context with the "check_structure". It'd be a better test if it built the PlanarEmbedding first and just had the check call inside the context. If that's too much for this PR we can put it in a separate one. But this touches that code already, so if you're up for it I'd appreciate that. Or let me know and I'll add it. :)
Co-authored-by: Dan Schult <dschult@colgate.edu>
…test for `add_half_edge()` (planarity.py, test_planarity.py)
…ising method call (test_planarity.py)
There was a problem hiding this comment.
Generally LGTM - I'm no expert on planar embeddings but this new API is certainly more intuitive.
@mdealencar will merging this interfere with #6798, or have these changes already been incorporated there? Either way I'm happy to push forward, just trying to get a handle on which changes are repeated where!
…ity.py) Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
I had already rebased #6798 to be applied after this one, as some changes here affect the implementation of new methods there. This PR stand by itself, though it does not resolve issue #6796, which is the motivation behind both PRs. I'm not sure if pushing only #6798 will carry the changes from here, but I think applying this first and then #6798 will leave a clearer record of the rationale behind the changes. |
There was a problem hiding this comment.
Thanks for the summary @mdealencar - in that case, let's get this one in and rebase #6798 on main once this has been merged.
Thanks for sticking it out through all the iterations and making review easier!
Add a new method for adding edges to PlanarEmbedding as an alternative to the existing three methods add..._cw, add..._ccw, and add..._first_edge. This tightens the API and allows for dropping explicit tracking of the `first_nbr` internal state variable. * feat: drop the use of node attribute "first_nbr" in PlanarEmbedding (planarity.py) * feat: add method PlanarEmbedding.add_half_edge(), which superceeds the _cw and _ccw varieties, plus update the planar_drawing and tests accordingly (planar_drawing.py, planarity.py, test_planarity.py) --------- Co-authored-by: mdealencar <mdealencar@users.noreply.github.com> Co-authored-by: Dan Schult <dschult@colgate.edu> Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
The use of a node attribute called
"first_nbr"makes it impossible to represent a valid PlanarEmbedding with a dict_of_dicts. This complicates the creation of valid and invalid embeddings for testing purposes, particularly if the changes in the PR to solve issue #6798 (which forbids the use ofPlanarEmbedding.add_edge*) are merged.The attribute
"first_nbr"seems to be relevant only toLRPlanarity.lr_planarity()and the way the methodsadd_half_edge_first()andadd_half_edge_ccw()update that attribute are quite unintuitive and undocumented. This means I doubt there is someone out there relying on this attributeThis PR introduces a new method
add_half_edge(), which covers the functionality ofadd_half_edge_cw(),add_half_edge_ccw()andadd_half_edge_first()while mimicking their behavior regarding the role of"first_nbr". These three methods are still fully functional, but could be deprecated in favor ofadd_half_edge()(although some form ofadd_half_edge_first()will be still necessary for LRPlanarity to work). The attribute"first_nbr"is no longer necessary and its functionality is obtained by relying on the keeping of insertion order of dict since Python 3.7.Tests related to planarity.py were updated to use the new method.