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
Various number field order and ideal utilities #4536
Comments
Attachment: trac-4536.patch.gz Based on 3.2.rc1 |
comment:1
Hi John, Just one quick comment. Is there a reason you are manually doing the caching instead of using the cached_method decorator in sage/misc/cachefunc.py? I think the result is a bit cleaner. Also, you don't need to use backslashes to continue lines if it occurs with in parens or brackets because Python knows that they need to be closed. --Mike |
comment:2
Replying to @mwhansen:
Quick answer: it never occurred to me to do it any other way! But isn't it completely standard in Sage that when an object has a property (such as the norm for an ideal) then one computes it the first time and caches it so that further requests for the property used the cached value? This is surely different from caching values of a function.
OK -- I'll remember that for next time (and if I get to revising this patch I'll remove them). Thanks
|
comment:3
Replying to @JohnCremona:
The cached_method decorator is relatively new which is why it isn't in use throughout Sage. For an example, see the groebner_basis method in sage/rings/polynomial/multi_polynomial_ideal.py That's exactly what the cached_method decorator does except that it also handles the case where arguments are passed into the method. The values are cached in a dictionary attribute on the object itself so it gets garbage collected correctly. It also supports things such as clearing the cache, etc. |
comment:4
Replying to @mwhansen:
That looks brilliant, and had completely passed me by. I'll start using it right away! It would also be a good idea to start to systematically use it all over (wouldn't it) -- then people would see it and use it themselves. |
Attachment: trac-4526-2.patch.gz |
comment:5
The second patch trac-4536-2.patch fixes a bug in the first implementation of residues(): I forgot to take the Smith Normal Form of the matrix. The first of the two new doctests is an example which failed with the old version. |
comment:7
Patches install and compile fine under 3.2, and all doctests in sage/rings/number_field pass. But I'm not happy with the is_coprime() method for fractional ideals. I thought the outcome of the discussion on the sage-nt list was that coprime for fractional ideals means disjoint supports, but I got this:
The problem here is that the fractional ideal j/k has norm 1, and the code falsely assumes that if norm(i) and norm(j) are coprime, then i and j must be coprime. Thus the code will say that j/k is coprime to everything (including itself). |
comment:8
David, you are quite right. We could just delete that coprimality of norms pre-test except when the ideals are integral. I'll correct it and put up a new patch. Thanks for spotting this mistake! |
comment:9
Attachment: trac-4536-fix.patch.gz The new patch trac-4536-fix.patch address the reviewer's just criticism, and adds a relevant doctest. |
comment:10
I've found no other problems, and the third patch certainly fixes the issue I reported, so I'll happily give this a positive review. |
comment:11
Merged all three patches in Sage 3.2.1.alpha1 |
Author: John Cremona |
Merged: 3.2.1.alpha1 |
Reviewer: David Loeffler |
Additional methods for (fractional) ideals:
Additional method for orders:
CC: @sagetrac-mtaranes dl267@cam.ac.uk
Component: number theory
Keywords: orders, ideals
Author: John Cremona
Reviewer: David Loeffler
Merged: 3.2.1.alpha1
Issue created by migration from https://trac.sagemath.org/ticket/4536
The text was updated successfully, but these errors were encountered: