-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A function checking if a tropical polytope is ordinarily convex (ie a polytrope) #3989
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3989 +/- ##
==========================================
+ Coverage 84.44% 84.56% +0.11%
==========================================
Files 596 596
Lines 81888 82074 +186
==========================================
+ Hits 69150 69404 +254
+ Misses 12738 12670 -68
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @Sami-Halaseh,
Thanks for the pull-request! Overall it looks great, I just have a few super-minor changes.
One general question: Wouldn't it be more sensible to make the input a polytope?
Best regards, Yue.
@YueRen thank you for the feedback. I have made the changes you suggested over a few commits (sorry about that). One thing I have not addressed is this comment you made. This is actually a confusion of mine. Are tropical polytopes a type in OSCAR? I thought the way do tropical convex hull computations was to somehow go through polymake. For example
Here it seems like I get two different types. The one when I pull $P into julia out of Polymake says something about type Polytope<Min, Rational>. But then the typeof gives something else. Excuse my ignorance here. At MEGA you suggested I speak with Michael Joswig about tropical convexity in OSCAR. I have reached out about a meeting so hopefully that will help soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @Sami-Halaseh,
The pull request looks great, maybe just two little changes to the docstring to make it clearer and it's good to be merged. If the changes works for you, you can just ask github to apply my suggestions (no need to manually change them yourself).
Best regards, Yue.
Co-authored-by: Yue Ren <[email protected]>
Co-authored-by: Yue Ren <[email protected]>
Hey Yue, thanks for the feedback. I think those suggestions are good and I have committed them. We are talking about a tropical polytope/polyhedron type but it looks like it will take some time to flesh out what it should look like and how to put it into OSCAR. For now I think its good for this function to work on matrices. |
It is kind of opposite the design of Oscar to have something like
For the same reason we don't have |
Then how about renaming the function to |
Maybe we can keep this function internal (non-exported) for now, until we have the tropical polytope type? Then we don't need to commit to a public interface function but it is still possible to be used for development. |
@benlorenz That's also a good solution (removing the export part of the changes). @Sami-Halaseh What do you say?
and wait for the tropical polytope type before having a public |
src/TropicalGeometry/matrix.jl
Outdated
pPMCV = P.POLYTOPE_MAXIMAL_COVECTORS | ||
Polymake.Shell.CV = pPMCV | ||
Polymake.shell_execute(raw"""$tmp = $CV->size;""") | ||
l = Polymake.Shell.tmp | ||
return l == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pPMCV = P.POLYTOPE_MAXIMAL_COVECTORS | |
Polymake.Shell.CV = pPMCV | |
Polymake.shell_execute(raw"""$tmp = $CV->size;""") | |
l = Polymake.Shell.tmp | |
return l == 1 | |
return length(P.POLYTOPE_MAXIMAL_COVECTORS) == 1 |
as above
Co-authored-by: Benjamin Lorenz <[email protected]>
Thank you for the feedback everyone. @YueRen I made those two changes. @benlorenz Thank you for the fix, the perl shell code was definitely not a good way to get the length. The wrapped type is much better, we just couldn't figure out how to do that properly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now needs the Oscar.
prefix until exported again.
Co-authored-by: Benjamin Lorenz <[email protected]>
Co-authored-by: Benjamin Lorenz <[email protected]>
Co-authored-by: Benjamin Lorenz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incorporating all suggestions! Looking forward to having polytropes in OSCAR, in the meantime this function is an excellent bandaid.
This is the wrong algorithm; must be replaced. Will be dealt with in #4001 . |
… polytrope) (oscar-system#3989) Co-authored-by: SamiHalaseh <[email protected]> Co-authored-by: Yue Ren <[email protected]> Co-authored-by: Benjamin Lorenz <[email protected]>
No description provided.