Opened 2 years ago
Closed 2 years ago
#26546 closed defect (fixed)
matrix_gfpn_dense: refactor field_to_int()
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  linear algebra  Keywords:  
Cc:  SimonKing  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Simon King 
Report Upstream:  N/A  Work issues:  
Branch:  3156293 (Commits)  Commit:  3156293eb39ab4f5c8278e583d19fe0a6724ad4d 
Dependencies:  Stopgaps: 
Description (last modified by )
sage: from sage.matrix.matrix_gfpn_dense import PrimeFieldConverter_class sage: C = PrimeFieldConverter_class(GF(5)) sage: C.field_to_int("foo")  ValueError Traceback (most recent call last) ValueError: invalid literal for int() with base 10: 'foo' Exception ValueError: "invalid literal for int() with base 10: 'foo'" in 'sage.matrix.matrix_gfpn_dense.PrimeFieldConverter_class.field_to_int' ignored 0
Maybe all calls to field_to_int
are done safely such that doesn't occur in practice. But still, it seems safer to have proper exception handling here.
I also noted that field_to_int
and int_to_field
are only ever used together with FfToInt
and FfFromInt
. So it makes a lot of sense to move those calls to FieldConverter_class
.
Change History (27)
comment:1 Changed 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Description modified (diff)
comment:3 in reply to: ↑ description ; followup: ↓ 4 Changed 2 years ago by
comment:4 in reply to: ↑ 3 ; followup: ↓ 5 Changed 2 years ago by
Replying to jdemeyer:
Simon, what you do think about this:
I also noted that
field_to_int
andint_to_field
are only ever used together withFfToInt
andFfFromInt
. So it makes a lot of sense to move those calls toFieldConverter_class
.
That seems to make sense.
If I see that correctly, field_to_int
actually returns nonnegative integers. Therefore, it would be easiest to impose except 1
in the function declaration.
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 2 years ago by
Replying to SimonKing:
If I see that correctly,
field_to_int
actually returns nonnegative integers. Therefore, it would be easiest to imposeexcept 1
in the function declaration.
My plan was to change this to take a FEL
as argument and return a FEL
and then use except? 255
. If you agree, I'll make that change.
comment:6 in reply to: ↑ 5 ; followup: ↓ 7 Changed 2 years ago by
Replying to jdemeyer:
Replying to SimonKing:
If I see that correctly,
field_to_int
actually returns nonnegative integers. Therefore, it would be easiest to imposeexcept 1
in the function declaration.My plan was to change this to take a
FEL
as argument and return aFEL
and then useexcept? 255
. If you agree, I'll make that change.
Are you sure that's how the function is used? It is supposed to be a conversion between good old Sage field elements (python objects!) and FEL.
In any case, if you have a function returning FEL, you can do except 255
without ?
, as the size of a field is at most 251.
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 2 years ago by
Replying to SimonKing:
Are you sure that's how the function is used? It is supposed to be a conversion between good old Sage field elements (python objects!) and FEL.
Yes, that's exactly what I meant: replace int
by FEL
.
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 2 years ago by
Replying to jdemeyer:
Replying to SimonKing:
Are you sure that's how the function is used? It is supposed to be a conversion between good old Sage field elements (python objects!) and FEL.
Yes, that's exactly what I meant: replace
int
byFEL
.
OK, as long as you do not want to change the input of field_to_int
or the output of int_to_field
, because that would clearly be a Sage object (element of GF(25,'x')
, for example).
Then, it should probably be called field_to_fel
and fel_to_field
. How could these be doc tested, as FEL is a C type?
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 2 years ago by
Replying to SimonKing:
How could these be doc tested, as FEL is a C type?
Well, int
is also a C type and you're already testing that...
comment:10 in reply to: ↑ 9 Changed 2 years ago by
Replying to jdemeyer:
Replying to SimonKing:
How could these be doc tested, as FEL is a C type?
Well,
int
is also a C type and you're already testing that...
Sure. Actually I thought it already is a problem if one renames a C type to some equivalent type. But if that's not a problem, then FEL certainly is the way to go.
comment:11 Changed 2 years ago by
 Branch set to u/jdemeyer/matrix_gfpn_dense__exceptions_ignored_in_field_to_int__
comment:12 Changed 2 years ago by
 Commit set to cda646e3374fa37268744b90ae7e53b124ff88cb
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cda646e  FieldConverter_class: new methods fel_to_field and field_to_fel

comment:13 Changed 2 years ago by
 Status changed from new to needs_review
comment:14 Changed 2 years ago by
 Summary changed from matrix_gfpn_dense: exceptions ignored in field_to_int() to matrix_gfpn_dense: refactor field_to_int()
comment:15 followup: ↓ 17 Changed 2 years ago by
What does "cdef readonly" mean?
comment:16 followup: ↓ 18 Changed 2 years ago by
This comment
.. WARNING:: Before calling the ``fel_to_field`` or ``field_to_fel`` methods, one should call the ``FfSetField`` function.
(appearing at least twice) looks potentially confusing. The user might ask, HOW to call FfSetField?
. Since it's a C library function, the user cannot call it, unless she writes Cython code.
comment:17 in reply to: ↑ 15 Changed 2 years ago by
Replying to SimonKing:
What does "cdef readonly" mean?
Accessible from Python, but readonly (contrary to cdef public
which is readwrite)
comment:18 in reply to: ↑ 16 Changed 2 years ago by
Replying to SimonKing:
Since it's a C library function, the user cannot call it, unless she writes Cython code.
The whole thing is only really useful for Cython anyway... but I see your point.
comment:19 Changed 2 years ago by
 Commit changed from cda646e3374fa37268744b90ae7e53b124ff88cb to 81808c073c1066087814f0768d1a8d9fbcf7930d
Branch pushed to git repo; I updated commit sha1. New commits:
81808c0  Mention that this is for Cython only

comment:20 Changed 2 years ago by
The changes look good to me and the tests at least in sage/matrix pass. Later I will run a complete test, and if that works it will be positive review.
comment:21 Changed 2 years ago by
 Reviewers set to Simon King
 Status changed from needs_review to positive_review
Tests pass, so, here is the previously announced positive review.
comment:22 followup: ↓ 23 Changed 2 years ago by
 Status changed from positive_review to needs_work
See patchbot
sage t long src/sage/matrix/matrix_gfpn_dense.pyx # 2 doctests failed
comment:23 in reply to: ↑ 22 Changed 2 years ago by
Replying to vbraun:
See patchbot
sage t long src/sage/matrix/matrix_gfpn_dense.pyx # 2 doctests failed
Right, that test lacks # optional: meataxe
. So, that test will fail if and only if meataxe is not installed (that's why that error didn't occur for me).
comment:24 Changed 2 years ago by
 Branch changed from u/jdemeyer/matrix_gfpn_dense__exceptions_ignored_in_field_to_int__ to u/SimonKing/matrix_gfpn_dense__exceptions_ignored_in_field_to_int__
comment:25 Changed 2 years ago by
 Commit changed from 81808c073c1066087814f0768d1a8d9fbcf7930d to 3156293eb39ab4f5c8278e583d19fe0a6724ad4d
 Status changed from needs_work to positive_review
I just noticed that the forgotten "# optional" still hasn't been added.
Fixed, tests pass now with and without optional meataxe. I suppose I can return to giving a positive review, as the new commit can be considered a "reviewer patch".
New commits:
3156293  Mark two test lines as optional

comment:26 Changed 2 years ago by
If it passes tests, sure.
comment:27 Changed 2 years ago by
 Branch changed from u/SimonKing/matrix_gfpn_dense__exceptions_ignored_in_field_to_int__ to 3156293eb39ab4f5c8278e583d19fe0a6724ad4d
 Resolution set to fixed
 Status changed from positive_review to closed
Simon, what you do think about this: