Opened 10 years ago

Closed 9 years ago

#8218 closed enhancement (fixed)

Finite Field move

Reported by: roed Owned by: davidloeffler
Priority: major Milestone: sage-4.4
Component: algebra Keywords: finite fields
Cc: robertwb Merged in: sage-4.4.alpha0
Authors: David Roe Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Moves all of the finite field files, the integer_mod files and the base classes from sage.rings.ring and sage.structure.element into their own folder in sage.rings. In preparation for more work on finite fields.

Attachments (7)

trac_8218_move.patch (671.0 KB) - added by roed 10 years ago.
Just moves the files
trac_8218_move.bundle (50.6 KB) - added by roed 10 years ago.
Bundle replacing trac_8218_move.patch. This should allow us to keep the repository information.
trac_8218_fixes.patch (214.9 KB) - added by roed 10 years ago.
Fixes the imports.
trac_8218_move_433.bundle (51.2 KB) - added by roed 10 years ago.
Rebased against 4.3.3: a bundle including all the moves; should be applied first
trac_8218_fixes_433.patch (219.0 KB) - added by roed 10 years ago.
Rebased against 4.3.3: should be applied after the bundle.
trac_8218_fixes_434alpha1.patch (220.2 KB) - added by davidloeffler 10 years ago.
apply after bundle (instead of trac_8218_fixes_433.patch)
trac_8218_doc.patch (864 bytes) - added by davidloeffler 10 years ago.
apply after bundle + trac_8218_fixes_434alpha1.patch

Download all attachments as: .zip

Change History (40)

Changed 10 years ago by roed

Just moves the files

comment:1 Changed 10 years ago by roed

  • Owner changed from AlexGhitza to roed

The files that are moved into sage.rings.finite_rings are:

integer_mod.pyx -> finite_rings/integer_mod.pyx
integer_mod.pxd -> finite_rings/integer_mod.pxd
integer_mod_ring.py -> finite_rings/integer_mod_ring.py
finite_field.py -> finite_rings/constructor.py
finite_field_prime_modn.py -> finite_rings/finite_field_prime_modn.py
finite_field_element.py -> finite_rings/element_ext_pari.py
finite_field_ext_pari.py -> finite_rings/finite_field_ext_pari.py
finite_field_givaro.pyx -> finite_rings/element_givaro.pyx
finite_field_givaro.pxd -> finite_rings/element_givaro.pxd
finite_field_ntl_gf2e.pyx -> finite_rings/finite_field_ntl_gf2e.pyx
finite_field_ntl_gf2e.pxd -> finite_rings/finite_field_ntl_gf2e.pxd
finite_field_morphism.py -> finite_rings/homset.py
part of ring.pyx -> finite_rings/finite_field_base.pyx
part of ring.pxd -> finite_rings/finite_field_base.pxd
part of sage/structure/element.pyx -> sage/rings/finite_rings/element_base.pyx
part of sage/structure/element.pxd -> sage/rings/finite_rings/element_base.pxd

comment:2 Changed 10 years ago by robertwb

It would be good to do this as a mercurial bundle, so that the history and other merges will follow correctly.

Changed 10 years ago by roed

Bundle replacing trac_8218_move.patch. This should allow us to keep the repository information.

comment:3 Changed 10 years ago by roed

  • Cc robertwb added
  • Status changed from new to needs_review

I created the bundle with

sage -hg bundle -r 13801 --base 13800 ~/patches-out/trac_8218_move.bundle

I believe this is right, but it's been a while since I used bundles.

comment:4 Changed 10 years ago by roed

  • Status changed from needs_review to needs_work

Oops. Forgot to fix pickles. Doing so now...

Changed 10 years ago by roed

Fixes the imports.

comment:5 Changed 10 years ago by roed

  • Status changed from needs_work to needs_review

Pickling should work now.

comment:6 Changed 10 years ago by drkirkby

  • Owner changed from roed to (none)

Has this been checked on Solaris? The patches are huge, so there is a good chance this will break on one platform or another.

There's general information about building on Solaris at

http://wiki.sagemath.org/solaris

Information specifically for 't2' at

http://wiki.sagemath.org/devel/Building-Sage-on-the-T5240-t2

Both the source (4.3.0.1 is the latest to build on Solaris) and a binary which will run on any SPARC can be found at http://www.sagemath.org/download-source.html

Dave

comment:7 Changed 10 years ago by drkirkby

  • Status changed from needs_review to needs_info

comment:8 Changed 10 years ago by robertwb

  • Status changed from needs_info to needs_review

The content, not the size, of the patches would determine whether there's any platform-dependent code here, and I highly doubt there is.

What I've looked at so far looks good (I started to do this myself once, but unfortunately there was some hitch and by the time I got to it again rebasing was neigh impossible). I hope to be able to get a review on this soon (just a matter of finding time).

comment:9 Changed 10 years ago by drkirkby

I do realise it is the content, not the size, but small innocuous looking patches have screwed up the build either on not only Solaris, but also Linux and OS X too.

It's difficult to me to see how we can know unless it is tested.

Changed 10 years ago by roed

Rebased against 4.3.3: a bundle including all the moves; should be applied first

comment:10 Changed 10 years ago by cremona

How do I apply a bundle? Can it be done in queues?

comment:11 Changed 10 years ago by roed

Within the deve/sage directory,

sage -hg unbundle trac_8218_move_433.bundle

comment:12 Changed 10 years ago by roed

Oh, and it can't be done in queues. If you want to work with a patch, use trac_8218_move.patch instead. I think that should apply; if not the only thing that patch or the bundle do is move a bunch of files (see a previous comment for the list). If one of the hunks fails, just use sage -hg rename.

comment:13 Changed 10 years ago by cremona

Sorry, still failing. After I unbundle the bundle I can neither run Sage nor rebuild it nor apply the "fix" patch, and have to delete the entire clone. The "move" patch does not apply to 4.3.3.

If you could post the exact sequence of commands needs to go from a fresh 4.3.3, starting with (say) sage -clone 8218 and ending with sage -br (or similar), then I'll willingly test it.

comment:14 follow-up: Changed 10 years ago by roed

Try the following (and let me know if it doesn't work):

cd $SAGE-ROOT
sage -clone 8218
cd devel/sage-8218/
sage -hg unbundle trac_8218_move.bundle
sage -hg merge
sage -hg commit -m "Merge"
sage -hg qinit
sage -hg qimport trac_8218_fixes_433.patch
sage -hg qpush
sage -br

comment:15 Changed 10 years ago by cremona

  • Status changed from needs_review to needs_work

OK, I did that (except with the bundle trac_8218_move_433.bundle), applying it all to 4.3.4.alpha0. The merge is ok (one piece of fuzz). The rebuild took a long time. But sage -br ended up not running properly:

AttributeError                            Traceback (most recent call last)

/home/john/sage-current/local/lib/python2.6/site-packages/IPython/ipmaker.pyc in force_import(modname)
     64         reload(sys.modules[modname])
     65     else:
---> 66         __import__(modname)
     67 
     68 

/home/john/sage-4.3.4.alpha0/local/bin/ipy_profile_sage.py in <module>()
      5     preparser(True)
      6 
----> 7     import sage.all_cmdline
      8     sage.all_cmdline._init_cmdline(globals())
      9 

/home/john/sage-current/local/lib/python2.6/site-packages/sage/all_cmdline.py in <module>()
     12 try:
     13 
---> 14     from sage.all import *
     15     from sage.calculus.predefined import x
     16     preparser(on=True)

/home/john/sage-current/local/lib/python2.6/site-packages/sage/all.py in <module>()
     70 get_sigs()
     71 
---> 72 from sage.rings.all      import *
     73 from sage.matrix.all     import *
     74 

/home/john/sage-current/local/lib/python2.6/site-packages/sage/rings/all.py in <module>()
     88 
     89 # Algebraic numbers

---> 90 from qqbar import (AlgebraicRealField, is_AlgebraicRealField, AA,
     91                    AlgebraicReal, is_AlgebraicReal,
     92                    AlgebraicField, is_AlgebraicField, QQbar,

/home/john/sage-current/local/lib/python2.6/site-packages/sage/rings/qqbar.py in <module>()
   1410 
   1411 # Cache some commonly-used polynomial rings

-> 1412 QQx = QQ['x']
   1413 QQx_x = QQx.gen()
   1414 QQy = QQ['y']

/home/john/sage-current/local/lib/python2.6/site-packages/sage/rings/ring.so in sage.rings.ring.Ring.__getitem__ (sage/rings/ring.c:2551)()

/home/john/sage-current/local/lib/python2.6/site-packages/sage/rings/polynomial/polynomial_ring_constructor.pyc in PolynomialRing(base_ring, arg1, arg2, sparse, order, names, name, implementation)
    341                 raise TypeError, "if second arguments is a string with no commas, then there must be no other non-optional arguments"
    342             name = arg1
--> 343             R = _single_variate(base_ring, name, sparse, implementation)
    344         else:
    345             # 2-4. PolynomialRing(base_ring, names, order='degrevlex'):


/home/john/sage-current/local/lib/python2.6/site-packages/sage/rings/polynomial/polynomial_ring_constructor.pyc in _single_variate(base_ring, name, sparse, implementation)
    421 
    422         elif base_ring.is_field(proof = False):
--> 423             R = m.PolynomialRing_field(base_ring, name, sparse, implementation=implementation)
    424 
    425         elif base_ring.is_integral_domain(proof = False):

/home/john/sage-current/local/lib/python2.6/site-packages/sage/rings/polynomial/polynomial_ring.pyc in __init__(self, base_ring, name, sparse, element_class, implementation)
   1194         if implementation is None: implementation="NTL"
   1195         if implementation == "NTL" and \
-> 1196                 sage.rings.finite_field.is_FiniteField(base_ring):
   1197             p=base_ring.characteristic()
   1198             from sage.libs.ntl.ntl_ZZ_pEContext import ntl_ZZ_pEContext

AttributeError: 'module' object has no attribute 'finite_field'
Error importing ipy_profile_sage - perhaps you should run %upgrade?
WARNING: Loading of ipy_profile_sage failed.

comment:16 Changed 10 years ago by roed

It looks like some additional patches got applied after what I based mine on... Sorry about that. I'll update the patch and post a new one soon.

This does illustrate why I'd like to get this reviewed. :-)

Changed 10 years ago by roed

Rebased against 4.3.3: should be applied after the bundle.

comment:17 in reply to: ↑ 14 Changed 10 years ago by roed

  • Status changed from needs_work to needs_review

You may need to clear out old files in the $SAGE_ROOT/devel/sage-8218/build directory.

comment:18 Changed 10 years ago by cremona

Sorry about this. I made a new clone of 4.3.4.alpha1 and followed the instructions to the letter, but the final qpush gave this:

applying trac_8218_fixes_433.patch
patching file sage/crypto/util.py
Hunk #1 FAILED at 31
1 out of 1 hunks FAILED -- saving rejects to file sage/crypto/util.py.rej
patching file sage/homology/chain_complex.py
Hunk #1 succeeded at 382 with fuzz 1 (offset 3 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_8218_fixes_433.patch

It might be better if someone who had any idea what they were doing took over this review -- I am clearly not competent! For a start, if I depart in the slightest way from the list of commands then it does not work at all, but I don't understand why.

Meanwhile I'll keep this clone in case sending any of the files will help (but despite the message here, there is nothing left in the "working directory").

comment:19 Changed 10 years ago by davidloeffler

  • Owner changed from (none) to davidloeffler

I managed to successfully build this under 4.3.4.alpha1. I had the same failure at the final qpush as John above, but I found the .rej file and merged it by hand. Build was fine, but time-consuming (why doesn't sage -b know about parallel processing?). I got just two doctest failures, one in /sage/crypto/public_key/blum_goldwasser.py and one in doc/en/reference/coercion.rst; both were trivial to fix.

I will upload a new patch replacing trac_8218_fixes_433.patch, which fixes the merge fuzz problems and corrects the two doctests.

Changed 10 years ago by davidloeffler

apply after bundle (instead of trac_8218_fixes_433.patch)

comment:20 follow-up: Changed 10 years ago by davidloeffler

  • Status changed from needs_review to positive_review

As for drkirkby's comment above: naturally it doesn't have a chance of building on 4.3.0.1 on T2. (I checked, just to make sure). I don't think it's reasonable to expect new patches to apply identically on two increasingly different forks of the Sage code base. (Isn't this what Mercurial's branching and merging tools are supposed to be for?)

comment:21 in reply to: ↑ 20 ; follow-up: Changed 10 years ago by drkirkby

Replying to davidloeffler:

As for drkirkby's comment above: naturally it doesn't have a chance of building on 4.3.0.1 on T2. (I checked, just to make sure). I don't think it's reasonable to expect new patches to apply identically on two increasingly different forks of the Sage code base. (Isn't this what Mercurial's branching and merging tools are supposed to be for?)

There are not two different forks of the code base. There are no Mercurial branches.

Be aware, given 4.3.4.alpha1 builds on Solaris, and passes all the doc tests, so I suspect if this breaks the build it will not be merged.

Dave

comment:22 in reply to: ↑ 21 Changed 10 years ago by davidloeffler

  • Status changed from positive_review to needs_work

Replying to drkirkby:

There are not two different forks of the code base. There are no Mercurial branches.

Be aware, given 4.3.4.alpha1 builds on Solaris, and passes all the doc tests, so I suspect if this breaks the build it will not be merged.

Ah! That's a totally different story then. Somehow I had got the incorrect impression that 4.3.0.1 was the latest Solaris version. I will run some tests on T2 and see what happens. Perhaps I had better do the same for the other tickets I have reviewed lately. My apologies!

David

comment:23 Changed 10 years ago by davidloeffler

  • Status changed from needs_work to needs_info

comment:24 Changed 10 years ago by davidloeffler

Aargh! 19 hours later, my copy of 4.3.4.alpha1 I was building on T2 for testing is still only half-compiled, and in the meantime 4.3.4.rc0 has come out which is going to break this again!

Changed 10 years ago by davidloeffler

apply after bundle + trac_8218_fixes_434alpha1.patch

comment:25 Changed 10 years ago by davidloeffler

  • Status changed from needs_info to needs_review

Here's a sneaky thing which I only discovered by accident: the reference manual index still points to the removed files in sage/rings that have been moved to sage/rings/finite_rings. This doesn't show up as an error if you install the bundle + patch and then build docs, because the old files are still floating around in the build directory.

I'm putting this back to "needs_review"; once I can get a fully working Sage running on T2 I will test it on that and put it back to "positive review" if it passes.

comment:26 follow-up: Changed 9 years ago by davidloeffler

  • Status changed from needs_review to positive_review

All seems to be well.

comment:27 in reply to: ↑ 26 Changed 9 years ago by cremona

Replying to davidloeffler:

All seems to be well.

David,

Can you list here exactly what needs to be applied (and how)? I want to start looking at the derivative patches for finite fields, so I have to get a clone with this one installed first -- and as you can see from earlier comments, I have not had much success so far.

comment:28 follow-up: Changed 9 years ago by davidloeffler

Hi John,

First apply the bundle trac_8218_move_433.bundle, using the commands:

hg unbundle http://trac.sagemath.org/sage_trac/raw-attachment/ticket/8218/trac_8218_move_433.bundle
hg merge
hg ci -m "merged finite field bundle"

Then use queues in the normal way to import the patches trac_8218_fixes_434alpha1.patch and trac_8218_doc.patch (in that order). This does work on 4.3.5, I just checked.

But the ball is in David Roe's court as far as the finite field patches are concerned. The sequence is #8218 --> #8332 -> #7880 -> #7883 -> #8333 -> #8334 -> #8335. So far

  • #8218 has a positive review
  • #7880 has a positive review
  • #7883 has been looked at by both me and Rob Bradshaw and we both agree that it needs more work.

Further downstream, #8333 builds independently of #7883, but many things in #8333 are horribly broken unless you also apply #8334, which does not build independently of #7883.

David

comment:29 in reply to: ↑ 28 Changed 9 years ago by cremona

Replying to davidloeffler:

Hi John,

...

David

Thanks! I saw that some of the later patches had had positive reviews too, so maybe it's a bit late for me to join in. But I thought that for a complicated interrelated set of patches like this, which non-trivial mathematical content, it would be good to have a small set of people looking at it rather than individuals.

comment:30 Changed 9 years ago by cremona

  • Status changed from positive_review to needs_work

I built this fine (following David L's instructions above). Rebuilding the docs produced this:

/home/john/sage-current/devel/sage/doc/en/reference/sage/rings/finite_field.rst:: WARNING: document isn't included in any toctree
/home/john/sage-current/devel/sage/doc/en/reference/sage/rings/finite_field_element.rst:: WARNING: document isn't included in any toctree
/home/john/sage-current/devel/sage/doc/en/reference/sage/rings/integer_mod.rst:: WARNING: document isn't included in any toctree
/home/john/sage-current/devel/sage/doc/en/reference/sage/rings/integer_mod_ring.rst:: WARNING: document isn't included in any toctree

Is this fixed later in the series? If not, it should be fixed here. So I am putting this back to "needs work".

comment:31 Changed 9 years ago by davidloeffler

  • Status changed from needs_work to needs_review

I had this too. I don't think it's a problem with the patch, it is because our documentation build system doesn't do the right thing when source files are removed and produces spurious warnings.

comment:32 Changed 9 years ago by davidloeffler

  • Status changed from needs_review to positive_review

comment:33 Changed 9 years ago by jhpalmieri

  • Authors changed from roed to David Roe
  • Merged in set to sage-4.4.alpha0
  • Resolution set to fixed
  • Reviewers set to David Loeffler
  • Status changed from positive_review to closed

Merged in 4.4.alpha0:

  • trac_8218_move_433.bundle
  • trac_8218_fixes_434alpha1.patch
  • trac_8218_doc.patch
Note: See TracTickets for help on using tickets.