#26546 closed defect (fixed)

matrix_gfpn_dense: refactor field_to_int()

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.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 jdemeyer)

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 14 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 14 months ago by jdemeyer

  • Description modified (diff)

comment:3 in reply to: ↑ description ; follow-up: Changed 14 months ago by jdemeyer

Simon, what you do think about this:

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.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 14 months ago by SimonKing

Replying to jdemeyer:

Simon, what you do think about this:

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.

That seems to make sense.

If I see that correctly, field_to_int actually returns non-negative integers. Therefore, it would be easiest to impose except -1 in the function declaration.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 14 months ago by jdemeyer

Replying to SimonKing:

If I see that correctly, field_to_int actually returns non-negative integers. Therefore, it would be easiest to impose except -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 ; follow-up: Changed 14 months ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

If I see that correctly, field_to_int actually returns non-negative integers. Therefore, it would be easiest to impose except -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.

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 ; follow-up: Changed 14 months ago by 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 by FEL.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 14 months ago by SimonKing

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 by FEL.

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 ; follow-up: Changed 14 months ago by 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...

comment:10 in reply to: ↑ 9 Changed 14 months ago by SimonKing

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.

Last edited 14 months ago by SimonKing (previous) (diff)

comment:11 Changed 14 months ago by jdemeyer

  • Branch set to u/jdemeyer/matrix_gfpn_dense__exceptions_ignored_in_field_to_int__

comment:12 Changed 14 months ago by git

  • Commit set to cda646e3374fa37268744b90ae7e53b124ff88cb

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cda646eFieldConverter_class: new methods fel_to_field and field_to_fel

comment:13 Changed 14 months ago by jdemeyer

  • Status changed from new to needs_review

comment:14 Changed 14 months ago by jdemeyer

  • Summary changed from matrix_gfpn_dense: exceptions ignored in field_to_int() to matrix_gfpn_dense: refactor field_to_int()

comment:15 follow-up: Changed 14 months ago by SimonKing

What does "cdef readonly" mean?

comment:16 follow-up: Changed 14 months ago by SimonKing

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 14 months ago by jdemeyer

Replying to SimonKing:

What does "cdef readonly" mean?

Accessible from Python, but read-only (contrary to cdef public which is read-write)

comment:18 in reply to: ↑ 16 Changed 14 months ago by jdemeyer

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 14 months ago by git

  • Commit changed from cda646e3374fa37268744b90ae7e53b124ff88cb to 81808c073c1066087814f0768d1a8d9fbcf7930d

Branch pushed to git repo; I updated commit sha1. New commits:

81808c0Mention that this is for Cython only

comment:20 Changed 14 months ago by SimonKing

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 14 months ago by SimonKing

  • 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 follow-up: Changed 13 months ago by vbraun

  • 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 13 months ago by SimonKing

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 13 months ago by SimonKing

  • 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 13 months ago by SimonKing

  • 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:

3156293Mark two test lines as optional

comment:26 Changed 13 months ago by jdemeyer

If it passes tests, sure.

comment:27 Changed 12 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.