Python interface for more parsing factors #25
Conversation
| index_siblings_.assign(length, vector<int>(length+1, -1)); | ||
| // If the arcs are to the left of a head token, they must be sorted by increasing | ||
| // modifier indices. If they are to the right, by decreasing indices. | ||
| void Initialize(const vector<Arc*> &arcs, const vector<Sibling*> &siblings) { |
There was a problem hiding this comment.
Could we maybe use overloading to keep the old API valid too? To avoid breaking code that relies on this.
Same goes for the python wrapper.
There was a problem hiding this comment.
Both functions kept for overloading
python/ad3/extensions.pyx
Outdated
| INT_DTYPE = np.int | ||
| FLOAT_DTYPE = np.float | ||
| ctypedef np.int_t INT_DTYPE_T | ||
| ctypedef np.float_t FLOAT_DTYPE_T |
There was a problem hiding this comment.
Is numpy used in this file for anything other than these typedefs? If not I think it is better to use the cython types rather than the numpy types here and avoid the import.
setup.py
Outdated
| @@ -71,7 +72,7 @@ def run(self): | |||
| language="c++"), | |||
| Extension("ad3.extensions", | |||
| ["python/ad3/extensions.cpp"], | |||
| include_dirs=[".", "ad3"], | |||
| include_dirs=[".", "ad3", np.get_include()], | |||
There was a problem hiding this comment.
same: if we can get away without a numpy build-time dependency it would be awesome; also revert the version number change please
python/ad3/extensions.pyx
Outdated
| @@ -256,12 +292,18 @@ cdef class PFactorHeadAutomaton(PGenericFactor): | |||
| if self.allocate: | |||
| del self.thisptr | |||
|
|
|||
| def initialize(self, int length, list siblings, bool validate=True): | |||
| def initialize(self, list arcs, list siblings, bool validate=True): | |||
There was a problem hiding this comment.
- remove type annotation from arcs so the users can pass an int instead
if isinstance(arcs, int):
arcs = [(i, j) for i in range... for j in range...]
# resume as usualThere was a problem hiding this comment.
Using arcs_or_length and keeping it backwards compatibly
removed numpy from cython file
|
The last commit also adds more flexibility to the |
|
Is this good to go now? |
|
hmm, I don't think |
| @@ -20,6 +20,7 @@ | |||
| #define FACTOR_HEAD_AUTOMATON | |||
|
|
|||
| #include "ad3/GenericFactor.h" | |||
| #include "FactorTree.h" | |||
There was a problem hiding this comment.
is this import needed? what for?
warning when installing with clang on macos
I implemented the Python interface for grandparents, grandsiblings and change the signature for siblings, making it arguably less error-prone. Also improved the documentation of some Cython functions.