Added arg checks (nrows/ncols) for constructors of ZZMatrix & QQMatrix

[JohnAAbbott]

Forbids negative matrix dimensions

[thofma]

Hm, these are internal and not meant for the user. We usually don't add argument checks for those. Because this also means checking things twice.

[codecov]

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 84.76%. Comparing base (22e3908) to head (a97df37).

❗ Current head a97df37 differs from pull request most recent head 69d1252. Consider uploading reports for the commit 69d1252 to get more accurate results

Files	Patch %	Lines
src/flint/FlintTypes.jl	90.69%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1711      +/-   ##
==========================================
- Coverage   84.93%   84.76%   -0.18%     
==========================================
  Files          95       94       -1     
  Lines       37235    37247      +12     
==========================================
- Hits        31627    31571      -56     
- Misses       5608     5676      +68     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JohnAAbbott]

I noticed that most of the constructors did not check the dimensions. The checks do need to be made at least for matrix_space specializations since these do seem to be publicly visible. It's not harmful to check the other constructors, but if you prefer to remove the checks, you can -- maybe an assertion is a possible compromise?

[thofma]

Do you remember which matrix_space or matrix invocation resulted in an error?

[JohnAAbbott]

Sorry for the delay in responding. I'm fairly it was just matrix_space(ZZ, -1,-1) or something very similar. An error was indeed signalled: flint reported "overflow", IIRC. Good that an error was signalled, but its nature was not so helpful.

[lgoettgens]

The problem is that one can create the matrix space without problems, but only gets an error when the first actual matrix is created. See

julia> M = matrix_space(ZZ, -1,-1)
Matrix space of -1 rows and -1 columns
  over integer ring

julia> zero(M)
Exception (flint). Overflow creating size -1 x -1 object.
ERROR: Problem in the Flint-Subsystem

[JohnAAbbott]

Thanks @lgoettgens for the explicit example; now I see it, I recall that that was indeed the problem I originally encountered.

[JohnAAbbott]

Presumably creating a matrix_space(ZZ, 0, -1) would not trigger overflow when creating an object of that space; but I think that an error ought to be signalled in this case too. My proposed modification produces:

julia> S = matrix_space(ZZ, 0,-1)
ERROR: Dimensions must be non-negative

[joschmitt]

I have the impression that the checks should then be put in the user facing matrix_space and matrix functions here and in AbstractAlgebra and not the constructors. Maybe this is in line with what @thofma said?

[lgoettgens]

I have the impression that the checks should then be put in the user facing matrix_space and matrix functions here and in AbstractAlgebra and not the constructors. Maybe this is in line with what @thofma said?

Yes, exactly.

[JohnAAbbott]

My preference is to leave the checks in the constructors:

1. the checks are very cheap (and may occasionally help with debugging)
2. since Julia does not offer "private" functions (as in C++), anyone could call the constructors
3. the checks might also help someone reading the code understand what restrictions the parameters must satisfy

Well, the code is not mine, so my preferences perhaps count for little...