Order polytope class #165
Order polytope class #165
Conversation
vaithak
force-pushed
the
order-polytope-class
branch
from
f0cb15b
to
e336f70
Compare
| ```bash | ||
| ./order_polytope poset_data.txt | ||
| ``` | ||
| where `poset_data.txt` is the file containing the poset data. |
There was a problem hiding this comment.
could you comment on how this file represents a poset?
There was a problem hiding this comment.
Added instructions regarding the representation of poset data in the file.
| #endif | ||
|
|
||
|
|
||
| template <typename Point> |
There was a problem hiding this comment.
does it make sense to inherit Hpolytope and then implement only the functions that are different for the order polytope?
There was a problem hiding this comment.
this can be done, but majority of the functions of the OrderPolytope class have different implementations compared to the HPolytope class. So, I think it won't be beneficial to inherit the Hpolytope class.
vaithak
force-pushed
the
order-polytope-class
branch
from
dec68fe
to
1cf37c0
Compare
| typedef Eigen::Matrix<NT, Eigen::Dynamic, Eigen::Dynamic> MT; | ||
|
|
||
| private: | ||
| Poset poset; |
There was a problem hiding this comment.
since you use "_" in the beginning of the name to declare a class member variable you should do it consistently in all member variables.
There was a problem hiding this comment.
I used "_" before those variables which are not core in representing the object, like for a poset, both n and order_relations are core for representing a poset. Similarly, in order_polytope only b is core for representation as A is not required in a matrix form, that's why _A is used. I used this because in hpolytope.h file, _d is used for representing the dimension variable.
There was a problem hiding this comment.
There is a lack of specific coding style in volesti and this is a problem we cannot solve here ;) So maybe it doesn't make sense to try to impose some style in the review.
My opinion is that it is a bit unclear to declare variables by whether or not they are "core".
FYI google style proposes a trailing underscore in all member variables https://google.github.io/styleguide/cppguide.html#Variable_Names
Maybe it is a good time to start a discussion on a coding style.
We can discuss this in an separate issue
include/misc/poset.h
Outdated
| RV order_relations; // pairs of form a <= b | ||
|
|
||
| public: | ||
| Poset(unsigned int _n, RV& _order_relations) : |
There was a problem hiding this comment.
this should be the other way around right? Use "_" in the beginning of the name to declare a class member variable and n, order_relations for input parameters of the constructor.
There was a problem hiding this comment.
commented the reasoning in the above comment.
|
|
||
|
|
||
| // verify if the relations are valid | ||
| static RV verify(const RV& relations, unsigned int n) |
There was a problem hiding this comment.
since you implemented this, it is a good practice to test it in tests
There was a problem hiding this comment.
currently the verify function does a very naive check of whether the relations have elements in [0, n-1]. I think adding a test for it will make sense when an addition check of cyclic relations are checked, which can be done in a future PR.
There was a problem hiding this comment.
still it would be useful to have a simple unit test for coverage.
| } | ||
|
|
||
| template <typename NT> | ||
| bool is_in(const std::vector<NT>& pt_coeffs, NT tol=NT(0)) const |
There was a problem hiding this comment.
this looks schematically weird. This is a class of a poset and this function checks if a point is in some other object, i.e. order polytope. Why not putting it inside order polytope instead?
There was a problem hiding this comment.
OK I see there is another is_in in order polytope, so now I wonder why is this function needed?
There was a problem hiding this comment.
the is_in function inside the poset class is only verifying whether an element verifies the order relations of the poset. It doesn't check whether the point will lie inside the order-polytope or not.
There was a problem hiding this comment.
Yes, I know, my question is if this function is needed. If needed then the name should be more descriptive e.g. satisfy_order(). Also, pt_coeffs refers to some kind of points but here we do not have points, maybe element is a better name. Lastly, there is not check that pt_coeffs's size and n are the same.
Implementation of order polytope class.