Skip to content

Replace reproducible_repr() with usage of displayhook#39420

Merged
vbraun merged 7 commits intosagemath:developfrom
user202729:reproducible-repr-notes
Feb 28, 2025
Merged

Replace reproducible_repr() with usage of displayhook#39420
vbraun merged 7 commits intosagemath:developfrom
user202729:reproducible-repr-notes

Conversation

@user202729
Copy link
Contributor

@user202729 user202729 commented Feb 1, 2025

There's a slight difference between reproducible_repr ordering (which always order by string representation) and this function (which tries first to order by element values, then by string representation), and the ordering of elements is reversed for multivariate polynomial ring (in R.<x,y,z> = QQ[], z < y < x). However, it should still be deterministic.

📝 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

@github-actions
Copy link

github-actions bot commented Feb 1, 2025

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

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

and the ordering of elements is reversed for multivariate polynomial ring (in R.<x,y,z> = QQ[], z < y < x)

Would it be desired/easy to change this to the more natural ordering x < y < z?


.. NOTE::

This function is mostly superseded by the automatic sorting
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we then mark it as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it's necessary. On the other hand it was never used except for one doctesting case, and letting it stay there might serve as a signpost to the correct handles of reproducible dict/set repr. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's no longer used/needed in the sage codebase, then I think it should be deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually there's a little caveat — if the repr forms a total order but the values themselves do not form a total order then it might be desirable to use this function (e.g. CIF in #39413 ). But I think it's rare enough that you can add an explicit sorted(…, key=[some reliable key]) anyway.

Copy link
Contributor Author

@user202729 user202729 Feb 6, 2025

Choose a reason for hiding this comment

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

Also there's still one place where the function is used below it (in trace_method which is also used in doctest):

        print("exit {} -> {}".format(meth, reproducible_repr(res)))

I think this usage is slightly more reasonable (it's not much longer than the alternative); on the other hand it can also be replaced with from IPython.lib.pretty import prettypretty(res).

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, if we can remove sage-specific things in the basic infrastructure then this is a big win. There is a considerably higher chance that people know about pretty from IPython than that they go hunting for a reproducible_repr method in sage. So if you think all the existing usage can be covered, then do please do so and deprecate the method.

sage: print(reproducible_repr(sols)) # needs sage.rings.polynomial.pbori
[{x: 0, y: 1, z: 0}, {x: 1, y: 1, z: 1}]
sage: sols # needs sage.rings.polynomial.pbori
[{z: 0, y: 1, x: 0}, {z: 1, y: 1, x: 1}]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here z < y < x while in the other examples a bit down it is still x < y < z?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, must be because I don't have the optional packages.

By the way, how can optional packages be installed in meson again? (sage -i doesn't work if I recalled correctly?) Find the corresponding conda package, but how do you know which conda package corresponds to which sage package?

Copy link
Contributor

Choose a reason for hiding this comment

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

Find the corresponding conda package, but how do you know which conda package corresponds to which sage package?

There are conda.txt files in build/pkgs that tell you the mapping from sage pkg to conda pkg. (But they are not actively maintained and thus may be incorrect.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

End up most of the packages needed seems not available on conda anyway (fes ginv macaulay2 magma msolve pycryptosat) so I just try to fix it by best effort.

@user202729
Copy link
Contributor Author

Would it be desired/easy to change this to the more natural ordering x < y < z?

Basically it is determined by Singular's monomial comparison (p_Cmp in singular_polynomial_cmp). If we just flip it, we get the unfortunate result of x^2 < x.

On the other hand we can flip the order we pass variables to Singular, which is technically possible, but it seems rather unnatural and the last time Singular polynomial comparison was changed a bunch of tests need to be fixed.

You can also argue that in R.<x,y>, then x has exponent vector (1, 0), y has exponent vector (0, 1), the latter is smaller. (which is probably how Singular implements the comparison internally.)

MPolynomialRing_polydict has the same behavior anyway.

If anything we should fix this upstream as well.

@user202729 user202729 marked this pull request as draft February 6, 2025 03:31
@user202729 user202729 marked this pull request as ready for review February 7, 2025 05:36
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 22, 2025
sagemathgh-39420: Replace reproducible_repr() with usage of displayhook
    
* It is not very well-known that doctest has magic features to ignore
the ordering of `dict` and `set` (see for example
sagemath#39413 (comment) ), I
try to make it more discoverable.
* Since the displayhook is in place (since
68b10e1), the `reproducible_repr`
(added in e68d6eb) is mostly redundant.
* Implement `_repr_pretty_` for `KeyConvertingDict` to make doctest
deterministic. (based on the framework of
sagemath#39027)

There's a slight difference between `reproducible_repr` ordering (which
always order by string representation) and this function (which tries
first to order by element values, then by string representation), and
the ordering of elements is reversed for multivariate polynomial ring
(in `R.<x,y,z> = QQ[]`, `z < y < x`). However, it should still be
deterministic.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39420
Reported by: user202729
Reviewer(s): Tobias Diez, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 24, 2025
sagemathgh-39420: Replace reproducible_repr() with usage of displayhook
    
* It is not very well-known that doctest has magic features to ignore
the ordering of `dict` and `set` (see for example
sagemath#39413 (comment) ), I
try to make it more discoverable.
* Since the displayhook is in place (since
68b10e1), the `reproducible_repr`
(added in e68d6eb) is mostly redundant.
* Implement `_repr_pretty_` for `KeyConvertingDict` to make doctest
deterministic. (based on the framework of
sagemath#39027)

There's a slight difference between `reproducible_repr` ordering (which
always order by string representation) and this function (which tries
first to order by element values, then by string representation), and
the ordering of elements is reversed for multivariate polynomial ring
(in `R.<x,y,z> = QQ[]`, `z < y < x`). However, it should still be
deterministic.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39420
Reported by: user202729
Reviewer(s): Tobias Diez, user202729
@vbraun vbraun merged commit ebb8fd6 into sagemath:develop Feb 28, 2025
20 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants