Skip to content
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

improve cone containment tests #30605

Closed
orlitzky opened this issue Sep 18, 2020 · 14 comments
Closed

improve cone containment tests #30605

orlitzky opened this issue Sep 18, 2020 · 14 comments

Comments

@orlitzky
Copy link
Contributor

If K is a polyhedral cone, the test x in K only supports x with rational coordinates. It is desirable in many cases to perform the same test with irrational numbers as well.

This will simplify the implementation in ticket #29169.

CC: @kliem @jplab @novoselt

Component: geometry

Author: Michael Orlitzky

Branch/Commit: 7ea2dd8

Reviewer: Jonathan Kliem

Issue created by migration from https://trac.sagemath.org/ticket/30605

@orlitzky orlitzky added this to the sage-9.2 milestone Sep 18, 2020
@novoselt
Copy link
Member

comment:1

I was thinking what should be done with reals and other non-exact things, but I think it does not matter - as long as exact ones are handled correctly, it is user's responsibility to be aware that non-exact things may give wrong results without extra care.

@orlitzky
Copy link
Contributor Author

comment:2

The difficulty is never where you think it will be...

sage: bool(I >= 0)                                                              
True

@orlitzky
Copy link
Contributor Author

comment:3

Maybe this suffices?


New commits:

cf36516Trac #30605: try AA before RR in cone._ambient_space_point().
276db84Trac #30605: catch more coercion failures in cone._ambient_space_point().
507affbTrac #30605: factor out repetition in cone._ambient_space_point().

@orlitzky
Copy link
Contributor Author

Branch: u/mjo/ticket/30605

@orlitzky
Copy link
Contributor Author

Commit: 507affb

@orlitzky
Copy link
Contributor Author

Author: Michael Orlitzky

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 19, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

09c2fa3Trac #30605: update polyhedral cone containment documentation.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 19, 2020

Changed commit from 507affb to 09c2fa3

@kliem
Copy link
Contributor

kliem commented Sep 23, 2020

Reviewer: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Sep 23, 2020

comment:6

Could you follow the recommendation of pycodestyle and do one command per line, e.g.

-        if p is not None: return p
+        if p is not None:
+            return p

or

-        try: return L.base_extend(ring)(data)
-        except TypeError: pass
-        except ValueError as ex:
-            if str(ex).startswith("Cannot coerce"): pass
+        try:
+            return L.base_extend(ring)(data)
+        except TypeError:
+            pass
+        except ValueError as ex:
+            if str(ex).startswith("Cannot coerce"):
+                pass

I think this make it easier to read.

Once done, you can set in on positive review on my behalf.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

7ea2dd8Trac #30605: reformat according to pycodestyle.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 23, 2020

Changed commit from 09c2fa3 to 7ea2dd8

@orlitzky
Copy link
Contributor Author

comment:8

Thanks! I fixed the indentation in one final (separate) commit if you want to take a look. But pycodestyle is happy now.

@vbraun
Copy link
Member

vbraun commented Sep 30, 2020

Changed branch from u/mjo/ticket/30605 to 7ea2dd8

@vbraun vbraun closed this as completed in ed7c4a3 Sep 30, 2020
orlitzky added a commit to orlitzky/sage that referenced this issue Apr 22, 2024
…30605.

Thanks to some recent improvements in cone containment testing, we no
longer have to reimplement it for vectors with irrational entries
ourselves.
orlitzky added a commit to orlitzky/sage that referenced this issue Apr 25, 2024
…30605.

Thanks to some recent improvements in cone containment testing, we no
longer have to reimplement it for vectors with irrational entries
ourselves.
orlitzky added a commit to orlitzky/sage that referenced this issue May 4, 2024
…30605.

Thanks to some recent improvements in cone containment testing, we no
longer have to reimplement it for vectors with irrational entries
ourselves.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants