Issue #33 - Move existing planarity module and associated files into classic subpackage, and add full subpackage with extended API for EAPS graphLib#35
Conversation
Note that this Cython sub-package has successfully been leveraged by refactored versions of the Test All Graphs Orchestrator and Edge Deletion Analyzer.
…g up the bounds-checking in the Graph methods - Removed EDGEFLAG_DIRECTION constants from cgraphLib.pxd because they aren't currently used in Graph class - Exposed gp_GetFirstEdge() and gp_EdgeInUseIndexBound(), then updated Graph.gp_IsArc() with additional bounds checks (also exposed gp_EdgeInUse()) - Added Graph.is_vertex() checks to Graph.gp_GetFirstArc() - Added checks to ensure valid edge index in Graph.gp_GetNextArc(), Graph.gp_EdgeInUse(), Graph.gp_GetNeighbor(), and Graph.gp_DeleteEdge() - Added checks to ensure valid link index in Graph.gp_AddEdge(), Graph.gp_DeleteEdge()
john-boyer-phd
left a comment
There was a problem hiding this comment.
This is great progress. There are a few smaller edits to make on this PR.
Seeing how it has come out, esp. for read/write iterator, suggests pulling in an updated release of EAPS graphLib, but that should be done as a separate issue on this repo.
- confirmed EDA invalid OK results for N=6-10 (see https://github.com/graph-algorithms/edge-addition-planarity-suite/blob/fcded21925346e68876d5e50124caa1483bb01a6/TestSupport/tables/) by grafting pre-105 fix files (i.e. commit 89ca75fa4a2f35357e4aed64ec837860f77f1878 before graph-algorithms/edge-addition-planarity-suite@54ded4d) into C source and locally compiling package - confirmed package published to TestPyPI produces 0 invalid OKs for N=6-10
john-boyer-phd
left a comment
There was a problem hiding this comment.
Your work here has prompted a very important reconsideration of the names of a few functions in the C graphLib, because of an inconsistency in naming. So, I am over there working on the issue now, and in the meantime the additional requested changes here will make the functions use a new naming convention that is consistent and generally behave as they currently do in the C graphLib (except that the inconsistency was gp_IsVertex(), and your code here has the consistent behavior, and the C graphLib is being corrected accordingly).
planarity/full/graph.pyx
Outdated
| def gp_IsVertex(self, int v): | ||
| return ( | ||
| (v >= self.gp_GetFirstVertex()) and | ||
| self.gp_VertexInRange(v) and |
There was a problem hiding this comment.
It would be better to use v<= self.gp_GetLastVertex() here, because of an upcoming change that appears to be necessary in the C API.
There was a problem hiding this comment.
Should I also remove the method gp_VertexInRange() from the Graph Cython class, as well as remove gp_VertexInRange() from the graph.pxd definition file?
planarity/full/graph.pyx
Outdated
|
|
||
| def is_vertex(self, int v): | ||
| return v >= self.gp_GetFirstVertex() and self.gp_VertexInRange(v) | ||
| def gp_IsVertex(self, int v): |
There was a problem hiding this comment.
It would be helpful to define another function gp_IsAnyVertex() that returns true if and only if v >= first vertex and v <= last virtual vertex. This will be needed because some parameter guard invocations of gp_IsVertex() need to be gp_IsAnyVertex() because they should work whether the call is related to a vertex or a virtual vertex. It would also be best for it to have a commented-out call to cgraphlib.gp_IsAnyVertex(v) with a FIXME comment to make it a little easier to add the call once the new version of graphLib shows up that contains the function.
There was a problem hiding this comment.
Should I be using the conjunct (v <= cgraphLib.gp_GetLastVirtualVertex(self._theGraph)) to test against the upper-bound for virtual vertices? So then the method would look something like:
def gp_IsAnyVertex(self, int v):
return (
(v >= self.gp_GetFirstVertex()) and
(v <= cgraphLib.gp_GetLastVirtualVertex(self._theGraph))
# FIXME: Add conjunct to call upcoming macro cgraphLib.gp_IsAnyVertex()
)
Should I go one step further and add a wrapper method for gp_GetLastVirtualVertex(theGraph)? If so, should I add a wrapper for gp_GetFirstVirtualVertex(theGraph) for symmetry? I don't want to clutter the API with wrappers that we don't yet use at the Python level, but the mixed syntax in the snippet above feels off D:
…ructures.h. Also updated second conjunct to explicitly test vertex index v against the index of the last vertex.
john-boyer-phd
left a comment
There was a problem hiding this comment.
As we hashed out in our last meeting, I agree that the methods calling gp_IsVertex() should continue to do so because we are not yet exposing any reason much less any API way to access or use the virtual vertices.
Resolves #33
Added
planarity/__init__.pxd,planarity/classic/__init__.pxd- Needed for Cython package discoverycimportstatements, as is done in__init__.py; see Cython Docs -pxdfiles:__init__.pxdplanarity/classic/__init__.py- supports imports fromclassicsubpackageplanarity/fullcappconst.pxd- definition file for constants defined inplanarity/c/graphLib/lowLevelUtils/appconst.hcgraphLib.pxd- definition file for constants and functions pertaining toGraphclass (currently used byplanarity/full/graph.pyxandplanarity/full/g6IterationUtils.pyxmodules)cg6IterationDefs.pxd- definitions fromplanarity/c/graphLib/io/g6-read-iterator.handplanarity/c/graphLib/io/g6-write-iterator.hrequired byplanarity/full/g6IterationUtils.pyxg6IterationUtils.pyx- introduces theG6ReadIteratorandG6WriteIteratorclasses, which wrap the functions implemented inplanarity/c/graphLib/io/g6-read-iterator.candplanarity/c/graphLib/io/g6-write-iterator.cthat operate overG6ReadIteratorPandG6WriteIteratorPrespectively (see EAPS Epic #10 for full context of this pattern)graph.pxd- Cython definition file that ensures we cancimporttheGraphfrom other Cython modules (e.g. theg6IterationUtilsmodule); from the Cython docs on pxd files:And from Sharing Extension types:
graph.pyx- introduces theGraphclass, which wraps agraphPand handles initialization and freeing in the__cinit__()and__dealloc__()methods, and wraps variousgp_functions and macros that operate ongraphPsg6IterationUtils.candgraph.c- extension source files produced when we Cythonize theplanarity.full.g6IterationUtilsandplanarity.full.graphExtensions for use when installing on platforms withoutcythoninstalledplanarity_app_utils.py- Pure python module containing constants and helper functions for working withGraphs, many of which have equivalents in the EAPSplanarityApplayerUpdated
MANIFEST.in- changedincludestatements to reflect newclassicsubpackage contents, and addedincludestatements forfullsubpackagepyproject.toml- added theplanarity.classicandplanarity.fullsubpackages to the explicit list of packages`setup.py- updated theExtensiondefinition for theplanarity.classic.planarity, and addedExtensions forplanarity.full.g6ReadIterationUtilsandplanarity.full.graphmodulesplanarity/__init__.py- Updated pathing forplanaritypackage'splanarityCython module, since it is now contained in theclassicsubpackage. Performed similar promotion offullAPI contents to theplanaritypackage namespaceplanarity/classic/cplanarity.pxd- updated pathing to reflect new project structureplanarity.pyx- updatedcimportstatement to reflect new project structurecplanarity.c- rebuilt Cython extension source file to be used when installing on platforms withoutcythoninstalledplanarity_functions.pyandplanarity_networkx.py- updated__all__lists of exposed functions for readability