Open roed314 opened 12 years ago
Attachment: 12053.patch.gz
Reviewing this I wondered why this is in the padics/
directory. Of course, the main application right now would be over the padics but it seems that everything would work like that for any DVR so should it probably be moved to rings/
?
What is reduce()
supposed to do? I don't understand that function.
This is a little alarming:
sage: ~R.ideal(5) * 5
Fractional ideal (5^-1) of 5-adic Field with capped relative precision 20
But I see fractional ideals are not implemented. Perhaps there should be a NotImplementedError
somewhere rather than manifestly incorrect behaviour. Is this fixed in a later patch?
I agree with saraedum's comment above, but for the moment it wouldn't hurt for it to live in padics/, especially as all that code is under heavy active development at the moment.
It wouldn't hurt to add some doctests to cover some unramified and ramified extensions of Zp (I tried a few myself and everything seems to work).
Apart from these issues the patch looks good to me.
I agree that ~R.ideal(5) * 5
is a bug. I'll try to fix it soon.
Description changed:
---
+++
@@ -1 +1,3 @@
Adds a new class for ideals in discrete valuation rings that improves speed and comparison.
+
+Prerequisite for #12077, #8240.
Branch: u/saraedum/develop
Changed branch from u/saraedum/develop to none
Not sure whether you still think that this code is useful…I turned it into a branch so it is easier to see what's going on.
Branch: u/saraedum/12053
Adds a new class for ideals in discrete valuation rings that improves speed and comparison.
Prerequisite for #12077, #8240.
Component: padics
Author: David Roe
Branch: u/saraedum/12053
Issue created by migration from https://trac.sagemath.org/ticket/12053