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

sage.numerical.interactive_simplex_method: improve support for base fields other than QQ and RDF #37865

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 24, 2024

We remove the ad-hoc conversion of data to RDF in InteractiveLPProblem.feasible_set. This is outdated and problematic:

  • the polyhedra code over RDF is not robust at all
  • Sage has good support for algebraic polyhedra, in particular via normaliz.

We also make the printing code in _latex_product more robust:

  • it no longer relies on SR to decide whether a coefficient needs to be wrapped in parens; instead, we use the same simple test that sage.misc.repr.coeff_repr already uses
  • recognizing trivial coefficients (0, 1) is switched to comparing text instead of comparing elements - in order to support fields such as SR where == is magic

Rebased version of:

Author: @mkoeppe, @ComboProblem

Resolves #31312.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Apr 24, 2024

Documentation preview for this PR (built with commit e5035be; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe mkoeppe force-pushed the t/31312/interactive_simplex_method__improve_support_for_base_fields_other_than_qq__rdf branch from 775dd97 to 998ccd4 Compare May 10, 2024 17:34
@mkoeppe mkoeppe marked this pull request as ready for review May 10, 2024 17:34
@mkoeppe mkoeppe changed the title sage.numerical.interactive_simplex_method: improve support for base fields other than qq rdf sage.numerical.interactive_simplex_method: improve support for base fields other than QQ and RDF May 10, 2024
@@ -301,22 +300,23 @@ def _latex_product(coefficients, variables,
sage: var("x, y") # needs sage.symbolic
(x, y)
sage: print(_latex_product([-1, 3], [x, y])) # needs sage.symbolic
- \mspace{-6mu}&\mspace{-6mu} x \mspace{-6mu}&\mspace{-6mu} + \mspace{-6mu}&\mspace{-6mu} 3 y
- \mspace{-6mu}&\mspace{-6mu} 1 x \mspace{-6mu}&\mspace{-6mu} + \mspace{-6mu}&\mspace{-6mu} 3 y
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ComboProblem I think this is an unintended change. Could you investigate what's causing this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is in Line 316-322. For the text input, with c=-1 and v = var("x"), we have that t.strip() = '-1' and c = 1. So the if block here isn't executed and the else block code writes a coefficient of c=1into the expression.

Furthermore, with fields that have a presentation of 1 that is not the string 1, we have that outputs are not as we'd expect for a coefficient of 1. See the attached example with RDF. Screenshot from 2024-05-14 11-07-58

For the -1 as a coefficient example, I think a change to line 316 like if t.strip() == '1' or t.strip() == '-1': would fix this. For fields with different presentations of 1 other than the string 1, I'm unsure of how to approach this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can live with 1.0 showing as 1.0; what's more surprising to me is that the objective constant shows as 0, not 0.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default constructor for InteractiveLPProblem specifies the objective_constant_term term as the integer 0 rather than the base ring element 0. The code doesn't explicitly convert the objective_constant_term to an element of the base ring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That should probably be changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just opened a PR that fixes this with the other changes discussed so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've merged your PR mkoeppe#41 now

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 10, 2024

@ComboProblem I've copied in the ticket description from the old ticket #31312. Part of what it's promising:

  • recognizing trivial coefficients (0, 1) is switched to comparing text instead of comparing elements - in order to support fields such as SR where == is magic

Could you add a test that uses an LP with coefficients from SR (for example, constant symbolic expressions such as pi or log(2) or sqrt(5))

@mkoeppe mkoeppe self-assigned this May 10, 2024
@mkoeppe mkoeppe force-pushed the t/31312/interactive_simplex_method__improve_support_for_base_fields_other_than_qq__rdf branch from e5835cc to a637158 Compare May 24, 2024 07:06
@mkoeppe mkoeppe force-pushed the t/31312/interactive_simplex_method__improve_support_for_base_fields_other_than_qq__rdf branch from a3bef0e to 61774e8 Compare May 25, 2024 18:10
@mkoeppe mkoeppe force-pushed the t/31312/interactive_simplex_method__improve_support_for_base_fields_other_than_qq__rdf branch from 61774e8 to b58c1c7 Compare June 2, 2024 00:21
@mkoeppe mkoeppe force-pushed the t/31312/interactive_simplex_method__improve_support_for_base_fields_other_than_qq__rdf branch from b58c1c7 to 20bbebf Compare June 9, 2024 17:52
@mkoeppe mkoeppe force-pushed the t/31312/interactive_simplex_method__improve_support_for_base_fields_other_than_qq__rdf branch from 20bbebf to 4dce510 Compare August 3, 2024 18:33
@mkoeppe mkoeppe force-pushed the t/31312/interactive_simplex_method__improve_support_for_base_fields_other_than_qq__rdf branch from 4dce510 to e5035be Compare October 25, 2024 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

interactive_simplex_method: Improve support for base fields other than QQ, RDF
2 participants