Opened 2 years ago

Closed 2 years ago

#25511 closed enhancement (fixed)

Clean up creating Matrix_gfpn_dense matrices

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.3
Component: linear algebra Keywords:
Cc: SimonKing Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw, Simon King
Report Upstream: N/A Work issues:
Branch: cb52ee3 (Commits) Commit: cb52ee3dcd416dffe224212100c1714957c9c22f
Dependencies: #25476, #25079, #25518 Stopgaps:

Description (last modified by jdemeyer)

  1. The class Matrix_gfpn_dense has a special (and undocumented!) constructor to create a matrix from a filename. This is a very special case because it is the only case where the first argument is not the parent (the matrix space). Because of this special case, some optimizations that I plan to do in #25505 become harder. Instead, this constructor from a filename is changed to a staticmethod Matrix_gfpn_dense.from_filename().
  1. Add a new function new_mtx to create a new Matrix_gfpn_dense from a meataxe Matrix_t* and use it where applicable. This simplifies a lot of existing code. It allows to remove _new() and many calls to __copy__.
  1. Remove self._cache = {} and self._is_immutable = False where possible because those shouldn't be needed.

Change History (33)

comment:1 in reply to: ↑ description Changed 2 years ago by SimonKing

Replying to jdemeyer:

So can we instead require that the first argument when constructing a matrix is always the parent? We can still allow creating a Matrix_gfpn_dense from a filename using a classmethod like Matrix_gfpn_dense.from_filename().

Basically the only reason is history: When I created p_group_cohomology-1.0 and the wrapper for MeatAxe used to be part of the spkg rather than the Sage library, I needed to be able to read matrices from a file created with one of the MeatAxe executables --- and I thought the easiest way is to provide the file to __init__. But any other way would be fine, too. And I agree that it would be a lot cleaner to not special case !Matrix_gfpn_dense.

This is phrased as a question because I could work around this issue if needed. However, if this filename constructor can be removed, that would make things simpler.

I would need to change a couple of places in p_group_cohomology (dunno how many places), but that would certainly be doable.

comment:2 Changed 2 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Summary changed from Can we get rid of the Matrix_gfpn_dense(filename) constructor? to Get rid of the Matrix_gfpn_dense(filename) constructor

comment:3 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Get rid of the Matrix_gfpn_dense(filename) constructor to Clean up creating Matrix_gfpn_dense matrices

comment:6 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:7 in reply to: ↑ description ; follow-up: Changed 2 years ago by SimonKing

  • Description modified (diff)

Replying to jdemeyer:

  1. The class Matrix_gfpn_dense has a special (and undocumented!)

I have an example for it in the doctests, but admittedly in the beginning of the docstring of __init__ it isn't mentioned.

  1. Add a new function new_from_meataxe to create a new Matrix_gfpn_dense from a meataxe Matrix_t* and use it where applicable.

That'd certainly be useful. IIRC I have such functions in my cohomology code, but certainly they'd belong here.

  1. Remove self._cache = {} and self._is_immutable = False where possible because those shouldn't be needed.

Why? Because Cython would automatically initialise a cdef dict attribute to an empty dict?

comment:8 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:9 in reply to: ↑ 7 ; follow-up: Changed 2 years ago by jdemeyer

Replying to SimonKing:

Why? Because Cython would automatically initialise a cdef dict attribute to an empty dict?

No, it is initialized to None which is sufficient.

comment:10 in reply to: ↑ 9 Changed 2 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

Why? Because Cython would automatically initialise a cdef dict attribute to an empty dict?

No, it is initialized to None which is sufficient.

Is it? I thought at some point I got an error when I failed to initialise it to an empty dict. But maybe I'm misremembering.

comment:11 follow-up: Changed 2 years ago by jdemeyer

  • Description modified (diff)

It is really important to support this:

            sage: M = Matrix_gfpn_dense.__new__(Matrix_gfpn_dense)   # optional: meataxe
            sage: N = copy(M)

I would prefer to raise an exception when copying an uninitialized matrix.

comment:12 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/can_we_get_rid_of_the_matrix_gfpn_dense_filename__constructor_

comment:14 Changed 2 years ago by jdemeyer

  • Commit set to 1029b04504c5173d3aecc4e0ae5f5d24d22ee16c
  • Status changed from new to needs_review

New commits:

1029b04Clean up creating Matrix_gfpn_dense matrices

comment:15 in reply to: ↑ 11 Changed 2 years ago by SimonKing

Replying to jdemeyer:

It is really important to support this:

            sage: M = Matrix_gfpn_dense.__new__(Matrix_gfpn_dense)   # optional: meataxe
            sage: N = copy(M)

I would prefer to raise an exception when copying an uninitialized matrix.

It's a corner case, I thought.

comment:16 follow-up: Changed 2 years ago by SimonKing

The code looks like a good simplification. But probably it won't compile (didn't check, though): Often you use a variable mat without cdef'ing it --- so, you cannot assign a Matrix_t* to it.

comment:17 Changed 2 years ago by jdemeyer

  • Dependencies set to #25476
  • Status changed from needs_review to needs_work

Conflicts with #25476.

comment:18 in reply to: ↑ 16 ; follow-up: Changed 2 years ago by jdemeyer

Replying to SimonKing:

Often you use a variable mat without cdef'ing it --- so, you cannot assign a Matrix_t* to it.

Cython can deal with that: it infers the type.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 2 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

Often you use a variable mat without cdef'ing it --- so, you cannot assign a Matrix_t* to it.

Cython can deal with that: it infers the type.

Seriously? Is that a recent addition? Because in the past I have often seen compiler errors when assigning a C pointer to something that isn't declared as such.

comment:20 Changed 2 years ago by git

  • Commit changed from 1029b04504c5173d3aecc4e0ae5f5d24d22ee16c to 63873901b7a954ad703071e0c45e5b49bcd2f780

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

4eae803Making matrices use the new _echelon_in_place method.
e2f0550Specify that _echelon_in_place shall return the pivots
6387390Clean up creating Matrix_gfpn_dense matrices

comment:21 Changed 2 years ago by jdemeyer

Also conflicts with #25079.

comment:22 in reply to: ↑ 19 Changed 2 years ago by jdemeyer

Replying to SimonKing:

Seriously? Is that a recent addition?

It's not recent, Cython has supported that for a long time.

Because in the past I have often seen compiler errors when assigning a C pointer to something that isn't declared as such.

Curious, maybe there was a different problem.

comment:23 Changed 2 years ago by jdemeyer

  • Dependencies changed from #25476 to #25476, #25079

comment:24 Changed 2 years ago by git

  • Commit changed from 63873901b7a954ad703071e0c45e5b49bcd2f780 to 5e09437599eb464a51d2e5e29b00bfc0a912a551

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

3c4a06dEnable _mul_long for matrices
5e09437Clean up creating Matrix_gfpn_dense matrices

comment:25 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:26 Changed 2 years ago by git

  • Commit changed from 5e09437599eb464a51d2e5e29b00bfc0a912a551 to 63e9d66610d2c2534b7864200d81271460ee5a7f

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

63e9d66Clean up creating Matrix_gfpn_dense matrices

comment:27 Changed 2 years ago by jdemeyer

  • Dependencies changed from #25476, #25079 to #25476, #25079, #25518

comment:28 Changed 2 years ago by git

  • Commit changed from 63e9d66610d2c2534b7864200d81271460ee5a7f to 13da208f0c7b640d8b644d2bcc41270e69a7c5bc

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

1eaed37More stuff in the meataxe interface, and a meataxe helper function
8a06c0fFix docstring formatting
13da208Clean up creating Matrix_gfpn_dense matrices

comment:29 Changed 2 years ago by git

  • Commit changed from 13da208f0c7b640d8b644d2bcc41270e69a7c5bc to cb52ee3dcd416dffe224212100c1714957c9c22f

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

cb52ee3Mark one doctest optional

comment:30 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM. Simon, if you have objects, feel free to revert.

comment:31 Changed 2 years ago by SimonKing

No objections.

Edit: I did look at the changeset introduced here, and it looks good to me. So, if all tests pass and Travis thinks it is good, then I not only have no objections but I support the positive review.

This ticket is the only dependency for #18514. So, I guess I can resume work there.

Last edited 2 years ago by SimonKing (previous) (diff)

comment:32 Changed 2 years ago by jdemeyer

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Simon King

comment:33 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/can_we_get_rid_of_the_matrix_gfpn_dense_filename__constructor_ to cb52ee3dcd416dffe224212100c1714957c9c22f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.