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

Error in UnionOfRows #39

Open
zickgraf opened this issue Feb 12, 2024 · 6 comments
Open

Error in UnionOfRows #39

zickgraf opened this issue Feb 12, 2024 · 6 comments

Comments

@zickgraf
Copy link
Member

The following example throws an error:

julia> using MatricesForHomalg

julia> Q = HomalgFieldOfRationals();

julia> UnionOfRows( HomalgZeroMatrix( 5, 10, QQ ), HomalgIdentityMatrix( 10, QQ ), HomalgZeroMatrix( 5, 10, QQ ) )
ERROR: MethodError: Cannot `convert` an object of type 
  Vector{Rational{BigInt}} to an object of type 
  AbstractAlgebra.Generic.MatSpaceElem

Closest candidates are:
  convert(::Type{T}, ::T) where T
   @ Base Base.jl:84

Stacktrace:
 [1] UnionOfRows(R::AbstractAlgebra.Generic.MatSpaceElem{Rational{BigInt}}, nr_cols::AbstractAlgebra.Generic.MatSpaceElem{Rational{BigInt}}, list::AbstractAlgebra.Generic.MatSpaceElem{Rational{BigInt}})
   @ MatricesForHomalg ~/.julia/dev/MatricesForHomalg/src/MatricesForHomalg.jl:861
 [2] top-level scope
   @ REPL[3]:1
@mohamed-barakat
Copy link
Member

Only the following is supported, also according to the documentation :)

UnionOfRows( QQ, 10, [ HomalgZeroMatrix( 5, 10, QQ ), HomalgIdentityMatrix( 10, QQ ), HomalgZeroMatrix( 5, 10, QQ ) ] )

@zickgraf
Copy link
Member Author

I see, thanks! MatrixCategory and CategoryOfRows use UnionOfRows with three matrices. Should UnionOfRows with several matrices be discouraged, or shall I turn this issue into a feature request?

@mohamed-barakat
Copy link
Member

MatrixCategory and CategoryOfRows use UnionOfRows with three matrices. Should UnionOfRows with several matrices be discouraged, or shall I turn this issue into a feature request?

I am for the former, since it carries more type information, something we generally encourage.

@mohamed-barakat
Copy link
Member

Could this issue be closed?

@zickgraf
Copy link
Member Author

I have not continued working on LinearAlgebraForCAP.jl, which is why this issue is still open. If we want to make porting code to Julia easy, the interfaces of MatricesForHomalg and MatricesForHomalg.jl should match as much as possible. If the interfaces differ, porting packages will always be a nuisance. That is, I suggest either implementing the convenience in MatricesForHomalg.jl or deprecating the convenience in MatricesForHomalg, if we want packages to be portable.

@zickgraf
Copy link
Member Author

If the interfaces differ, porting packages will always be a nuisance.

I now experience this while porting ModulePresentationsForCAP: there are more than 30 occurrences of UnionOfRows and UnionOfColumns. Converting all of them is time-consuming and error-prone, so I will ignore those parts of the code for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants