Opened 2 years ago

Closed 2 years ago

#23192 closed enhancement (fixed)

some_elements is trivial for orders/number fields

Reported by: saraedum Owned by:
Priority: major Milestone: sage-8.0
Component: number fields Keywords: sd86.5, sd87
Cc: Merged in:
Authors: Edouard Rousseau, Julian Rüth Reviewers: Julian Rüth, Aly Deines
Report Upstream: N/A Work issues:
Branch: ed0ae9c (Commits) Commit: ed0ae9c7520ce47d41e268e5aa4eddb89856ba9a
Dependencies: Stopgaps:

Description (last modified by saraedum)

Currently this is trivial

sage: G = GaussianIntegers()
sage: G.some_elements()
[1]

The changes introduced here, make a problem with gcd/xgcd in number fields visible, see #23274. This is however not an issue introduced by this ticket, so it is not within the scope of this ticket to fix it (and not even a dependency imho.)

Change History (18)

comment:1 Changed 2 years ago by erousseau

  • Branch set to u/erousseau/some_elements_is_trivial_for_orders_number_fields

comment:2 Changed 2 years ago by erousseau

  • Authors set to Edouard Rousseau
  • Commit set to bfafce31cecd2a79d6aafb02d2dd19a698ed81b8
  • Status changed from new to needs_review

I tried to choose "typical" elements, please let me know if you think this is not done in a good way.

Edouard


New commits:

bfafce3Wrote some_elements for the classes Order and NumberField.

comment:3 follow-up: Changed 2 years ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to needs_work

This looks already quite good. A few minor comments (sorry that it's so many…nothing of this is really important):

  1. Many places in the documentation talk about self but it is usually best to avoid this (because people who don't know Python, don't understand it - and it also sounds unnatural.) So it is better to talk about "this number field" and "this order" instead.
  2. For a similar reason, you should not mention the parameter self in the INPUT section. From a user's perspective, these methods take no parameters, so the INPUT section can go away.
  3. Since the OUTPUT section does not say anything different from the first line of the docstring, you could as well just leave it out, but it also does not hurt.
  4. In docstrings, it is usually considered more readable, to write t^2 - 2 instead of t^2-2, i.e., use spaces around + and -.
  5. I would probably not print the polynomial rings, since their not of interest here.
  6. Is length ever == 1?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 2 years ago by erousseau

Replying to saraedum:

Is length ever == 1?

I don't know if we want to consider cases like that, but the user can define degenerate extensions such as

sage: R.<t> = QQ[]
sage: K.<a> = QQ.extension(t)
sage: Z = K.ring_of_integers()
sage: Z.basis()
[1]

that is why I added that test.

Anyway, I will change the rest, and that test too if you think that it is irrelevant.

Edouard

comment:5 in reply to: ↑ 4 Changed 2 years ago by saraedum

Replying to erousseau:

Replying to saraedum: Anyway, I will change the rest, and that test too if you think that it is irrelevant.

Oh, I did not know that this was "allowed". You should definitely leave the test in then. You could probably add a doctest for this, saying something like

TESTS:

This also works for trivial extensions::

sage: ... your example ...

to make this clear for anybody who stumbles upon this.

comment:6 Changed 2 years ago by git

  • Commit changed from bfafce31cecd2a79d6aafb02d2dd19a698ed81b8 to ecf6a0ea90030c79d19a25fb9ba9d761133e9a79

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

ecf6a0eChanged the doctest and added a test to some_element in the class NumberField to deal with trivial extensions.

comment:7 Changed 2 years ago by erousseau

  • Status changed from needs_work to needs_review

I tried to follow your remarks, please let me know if I missed something.

Also, I added a test to deal with the trivial extensions in the class NumberField.

Edouard

comment:8 Changed 2 years ago by saraedum

  • Status changed from needs_review to needs_work

I have not tried this, but I think that the absolute_degree does not matter. Currently, you would miss a trivial extension of a non-trivial extension (and produce a division by zero if the second extension has gen == 1). Or am I missing something?

comment:9 Changed 2 years ago by erousseau

Yes you are right, currently I would divide by zero if one of the extensions is trivial with a generator equal to 0 or -1. But if I use relative_degree, I can still divide by zero if the last extension is not trivial but there is a trivial one in the tower.

One thing I can do is just test gen == 0 and add only gen to the lists of elements if it is zero, otherwise add gen and gen**(-1).

But then, since gen can be anything, I can add some elements two times, e.g. if gen is 1. It is probably not a big problem...

comment:10 Changed 2 years ago by saraedum

Ok. I underestimated the complexity of this.

So, there's is also a completely different approach to this, basically along the lines of https://git.sagemath.org/sage.git/diff?id2=d9e145d44c3e2e18e00409ff93620d4bee6141a2&id=61b3a437fdcd9935a9e4e0b7b07b1ae2b0e30ff0. So, for number fields, you could take a, b in the underlying polynomial ring, plug in the generator of the field and create the quotient if it makes sense. For orders, you could take elements in the number field and filter out those that are in the order.

comment:11 Changed 2 years ago by saraedum

  • Branch changed from u/erousseau/some_elements_is_trivial_for_orders_number_fields to u/saraedum/some_elements_is_trivial_for_orders_number_fields

comment:12 Changed 2 years ago by saraedum

  • Commit changed from ecf6a0ea90030c79d19a25fb9ba9d761133e9a79 to db6798b10aa164c1366fcdb31825e16bbbe87fa1
  • Description modified (diff)

New commits:

db6798bMerge branch 'develop' into t/23192/some_elements_is_trivial_for_orders_number_fields

comment:13 Changed 2 years ago by git

  • Commit changed from db6798b10aa164c1366fcdb31825e16bbbe87fa1 to ed0ae9c7520ce47d41e268e5aa4eddb89856ba9a

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

ed0ae9cgeneric some_elements() for number fields

comment:14 Changed 2 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:15 Changed 2 years ago by saraedum

  • Authors changed from Edouard Rousseau to Edouard Rousseau, Julian Rüth

comment:16 Changed 2 years ago by roed

  • Keywords sd87 added

comment:17 Changed 2 years ago by aly.deines

  • Reviewers changed from Julian Rüth to Julian Rüth, Aly Deines
  • Status changed from needs_review to positive_review

This looks good, though I think it's a little weird that the list length isn't a parameter (but it makes sense that it isn't given the code.)

comment:18 Changed 2 years ago by vbraun

  • Branch changed from u/saraedum/some_elements_is_trivial_for_orders_number_fields to ed0ae9c7520ce47d41e268e5aa4eddb89856ba9a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.