Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

matrixobj: deal with mutability questions #5254

Merged
merged 4 commits into from
Dec 18, 2022

Conversation

ThomasBreuer
Copy link
Contributor

(This addresses issue #5231.)

I hope this proposal makes sense. Comments are welcome.

  • changed the mutability definition for vector and matrix objects:

    • The ConstructingFilter value indicates whether mutable variants are possible at all, as follows: If the filter implies IsCopyable then one can create mutable variants, otherwise not.
    • In the definitions of NewVector, ZeroVector, NewMatrix, ZeroMatrix, IdentityMatrix, promise (fully) mutable results if and only if the filter in question implies IsCopyable.
    • In the definitions of Vector and Matrix, promise mutable results if and only if the filter in question implies IsCopyable and the vector/matrix that defines the entries is mutable.
  • changed the currently available ConstructingFilter values such that they imply IsCopyable. (I assume that these filters can be found as follows: flags:= List( [ IsVectorObj, IsMatrixObj ], FLAG1_FILTER );; Filtered( FILTERS, x -> IsRepresentation( x ) and Intersection( flags, TRUES_FLAGS( WITH_IMPS_FLAGS( FLAGS_FILTER( x ) ) ) ) <> [] ); )

  • extended the definition of Vector( v, w ) such that also the case of w in IsPlistRep is supported, and added a method for both arguments in IsList and IsCyclotomicCollection (to be used for example for objects in IsAlgebraicElement).

- changed the mutability definition for vector and matrix objects:
  - The `ConstructingFilter` value indicates whether mutable variants
    are possible at all, as follows:
    If the filter implies `IsCopyable` then one can create mutable variants,
    otherwise not.
  - In the definitions of `NewVector`, `ZeroVector`, `NewMatrix`, `ZeroMatrix`,
    `IdentityMatrix`, promise (fully) mutable results if and only if the
    filter in question implies `IsCopyable`.
  - In the definitions of `Vector` and `Matrix`, promise mutable results
    if and only if the filter in question implies `IsCopyable` and the
    vector/matrix that defines the entries is mutable.

- changed the currently available `ConstructingFilter` values such that
  they imply `IsCopyable`.
  (I assume that these filters can be found as follows.)
  ```
  flags:= List( [ IsVectorObj, IsMatrixObj ], FLAG1_FILTER );;
  Filtered( FILTERS, x -> IsRepresentation( x ) and Intersection( flags, TRUES_FLAGS( WITH_IMPS_FLAGS( FLAGS_FILTER( x ) ) ) ) <> [] );
  ```

- extended the definition of `Vector( v, w )` such that also the case of
  `w` in `IsPlistRep` is supported,
  and added a method for both arguments in `IsList and IsCyclotomicCollection`
  (to be used for example for objects in `IsAlgebraicElement`).
@ThomasBreuer ThomasBreuer added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Dec 7, 2022
@fingolfin fingolfin changed the title deal with mutability questions matrixobj: deal with mutability questions Dec 8, 2022
... of vector and matrix objects, and mention that they admit
mutable variants

Details:

- moved the manual section "Basic operations for row/column reductions"
  above the section "Implementing New Vector and Matrix Objects Types",
  in order to collect the stuff about implementations at the end of the
  chapter

- added new manual sections for the documentation of
  `IsGF2VectorRep`, `Is8BitVectorRep`, `IsZmodnZVectorRep`,
  `IsGF2MatrixRep`, `Is8BitMatrixRep`, `IsZmodnZMatrixRep`

- moved the documentation of `IsPlistVectorRep` and
  `IsPlistMatrixRep` to the new sections

- changed some formulations such that `IsPlistMatrixRep` appears
  just as one instance of `IsRowListMatrix`
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed I am overall very happy with this except for the low-level implementation details (see comment). Thomas is working on this; once resolved we can merge!

## a matrix object (see <Ref Filt="IsMatrixObj"/>) that behaves like the
## list of its rows (see <Ref Filt="IsRowListMatrix"/>).
## <A>obj</A> is internally represented as a positional object
## (see <Ref Filt="IsPositionalObjectRep"/>) with <M>4</M> entries.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we just discussed on gather.town, I'd prefer to either keep these implementation specifics out of the user-visible documentation, or else to at least qualify them to indicate that these are subject to change and one should not write code exploiting them but rather use official interfaces.

... from the documentation of representations of matrix and vector objects.

(This affects documentation added in a previous commit of this
pull request as well as documentation that has been available
already before.)
@ThomasBreuer
Copy link
Contributor Author

@fingolfin I have addressed your request.
(Somehow your comment did not get marked as "outdated": The lines in question are still there, but they were moved away from the block that is used for the documentation.)

@fingolfin fingolfin merged commit eef45e1 into gap-system:master Dec 18, 2022
@ThomasBreuer ThomasBreuer deleted the TB_MatrixObj_mutability branch December 19, 2022 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants