Opened 4 years ago
Closed 4 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, GitHub, GitLab) | Commit: | ed0ae9c7520ce47d41e268e5aa4eddb89856ba9a |
Dependencies: | Stopgaps: |
Description (last modified by )
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 4 years ago by
- Branch set to u/erousseau/some_elements_is_trivial_for_orders_number_fields
comment:2 Changed 4 years ago by
- Commit set to bfafce31cecd2a79d6aafb02d2dd19a698ed81b8
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 4 years ago by
- 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):
- 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. - 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. - 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.
- In docstrings, it is usually considered more readable, to write
t^2 - 2
instead oft^2-2
, i.e., use spaces around+
and-
. - I would probably not print the polynomial rings, since their not of interest here.
- Is
length
ever== 1
?
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
- Commit changed from bfafce31cecd2a79d6aafb02d2dd19a698ed81b8 to ecf6a0ea90030c79d19a25fb9ba9d761133e9a79
Branch pushed to git repo; I updated commit sha1. New commits:
ecf6a0e | Changed the doctest and added a test to some_element in the class NumberField to deal with trivial extensions.
|
comment:7 Changed 4 years ago by
- 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 4 years ago by
- 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 4 years ago by
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 4 years ago by
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 4 years ago by
- 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 4 years ago by
- Commit changed from ecf6a0ea90030c79d19a25fb9ba9d761133e9a79 to db6798b10aa164c1366fcdb31825e16bbbe87fa1
- Description modified (diff)
New commits:
db6798b | Merge branch 'develop' into t/23192/some_elements_is_trivial_for_orders_number_fields
|
comment:13 Changed 4 years ago by
- Commit changed from db6798b10aa164c1366fcdb31825e16bbbe87fa1 to ed0ae9c7520ce47d41e268e5aa4eddb89856ba9a
Branch pushed to git repo; I updated commit sha1. New commits:
ed0ae9c | generic some_elements() for number fields
|
comment:14 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:15 Changed 4 years ago by
comment:16 Changed 4 years ago by
- Keywords sd87 added
comment:17 Changed 4 years ago by
- 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 4 years ago by
- 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
I tried to choose "typical" elements, please let me know if you think this is not done in a good way.
Edouard
New commits:
Wrote some_elements for the classes Order and NumberField.