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

normalize_coordinates for projective morphisms gcd failure #35797

Closed
2 tasks done
bhutz opened this issue Jun 19, 2023 · 6 comments · Fixed by #35809
Closed
2 tasks done

normalize_coordinates for projective morphisms gcd failure #35797

bhutz opened this issue Jun 19, 2023 · 6 comments · Fixed by #35809

Comments

@bhutz
Copy link

bhutz commented Jun 19, 2023

Is there an existing issue for this?

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.

Did you read the documentation and troubleshoot guide?

  • I have read the documentation and troubleshoot guide

Environment

- **OS**: Ubuntu 20.04
- **Sage Version**: 10.0

Steps To Reproduce

K.<a> = NumberField(3*x^2 + 1)
P.<z,w> = ProjectiveSpace(K, 1)
phi = DynamicalSystem_projective([a*(z^2 + w^2),z*w])
phi.normalize_coordinates()

Expected Behavior

Should return a version of the point with normalized coordinates

Actual Behavior

the gcd step fails since it assumes clearing denominators gives back points in the ring of integers.

Additional Information

No response

@bhutz bhutz added this to the sage-10.1 milestone Jun 19, 2023
@DaveWitteMorris
Copy link
Member

Putting the "Steps to reproduce" into sage gives syntax errors, because it is not displaying properly. Please surround the code block with triple backticks.

Also, please delete "<title>" from the end of the title.

guojing0 added a commit to guojing0/sage that referenced this issue Jun 20, 2023
…turn a version of the point with normalized coordinates
@guojing0 guojing0 self-assigned this Jun 20, 2023
@bhutz bhutz changed the title normalize_coordinates for projective morphisms gcd failure<title> normalize_coordinates for projective morphisms gcd failure Jun 22, 2023
@videlec
Copy link
Contributor

videlec commented Jun 26, 2023

Hello. There are two errors rather than one I would say

  • the problematic bit is that when K is a number field, normalize_coordinates tries to be "smart" using gcd
  • when computing gcd over number fields, ultimately there is a following conversion failure (see below)

The conversion problem on number fields is

sage: K.maximal_order()(K(a))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-5-89ae56cf6326> in <module>
----> 1 K.maximal_order()(K(a))

/usr/lib/python3/dist-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9458)()
    896         if mor is not None:
    897             if no_extra_args:
--> 898                 return mor._call_(x)
    899             else:
    900                 return mor._call_with_args(x, args, kwds)

/usr/lib/python3/dist-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4735)()
    159                 print(type(C), C)
    160                 print(type(C._element_constructor), C._element_constructor)
--> 161             raise
    162 
    163     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/usr/lib/python3/dist-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4627)()
    154         cdef Parent C = self._codomain
    155         try:
--> 156             return C._element_constructor(x)
    157         except Exception:
    158             if print_warnings:

/usr/lib/python3/dist-packages/sage/rings/number_field/order.py in _element_constructor_(self, x)
   1239         V, _, embedding = self._K.vector_space()
   1240         if not embedding(x) in self._module_rep:
-> 1241             raise TypeError("Not an element of the order.")
   1242         return self._element_type(self, x)
   1243 

TypeError: Not an element of the order.

@bhutz
Copy link
Author

bhutz commented Jun 26, 2023

Yes, the problem is that simply clearing denominators is not enough to ensure you are in the ring of integers (3a rather than a is the integral element). So I think the fix here is to be more careful in moving to the ring of integers.

@bhutz
Copy link
Author

bhutz commented Jun 26, 2023

btw, there is a pull request made for this (35809), but it doesn't seem to be attached to this issue. How do those two get connected?

@DaveWitteMorris
Copy link
Member

See the github documentation on linking a PR to an issue. Slightly changing the description of the PR should be enough. I'm not an expert, but instead of "fix issue #35797", I think it should say "fixes #35797". The manual approach would be to click on "Development" in the right-hand column of the PR web page, and add this issue. Either of these methods would need to be done by the author of the PR (or someone else with write access).

@guojing0
Copy link
Contributor

guojing0 commented Jun 26, 2023

See the github documentation on linking a PR to an issue. Slightly changing the description of the PR should be enough. I'm not an expert, but instead of "fix issue #35797", I think it should say "fixes #35797". The manual approach would be to click on "Development" in the right-hand column of the PR web page, and add this issue. Either of these methods would need to be done by the author of the PR (or someone else with write access).

Thank you. I did the first method ("fixes #35797"), and somehow Github automatically did the second for me.

guojing0 added a commit to guojing0/sage that referenced this issue Jun 27, 2023
vbraun pushed a commit that referenced this issue Jul 1, 2023
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

This PR also attempts to address and fixes #35797.

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [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 accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35809
Reported by: Jing Guo
Reviewer(s): bhutz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants