AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

Add ones for NCRing

Open fingolfin opened this issue 1 year ago • 4 comments

Also simplify zeros implementation and add some tests

fingolfin avatar Oct 30 '24 13:10 fingolfin

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.13%. Comparing base (475eafc) to head (b03d06d). Report is 184 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1886      +/-   ##
==========================================
- Coverage   88.13%   88.13%   -0.01%     
==========================================
  Files         120      120              
  Lines       30286    30281       -5     
==========================================
- Hits        26694    26689       -5     
  Misses       3592     3592              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 30 '24 13:10 codecov[bot]

Hrm, I am torn on this.

We "usually don't want that" for our own matrices, but it is what ones and zeros does in Julia, e.g.

julia> M = ones(BigInt, 3, 3)
3×3 Matrix{BigInt}:
 1  1  1
 1  1  1
 1  1  1

julia> M[1,1] === M[1,2]
true

Of course this is not practical for actual use within OSCAR, but that's not what this is about.

Perhaps this is an argument to turn both of these functions into just error like error("please use zero_matrix instead") or so.

fingolfin avatar Oct 30 '24 21:10 fingolfin

I don't consider it a problem that all elements are identical.

thofma avatar Oct 30 '24 21:10 thofma

We just had someone on slack that was trapped by the julia behavior. Of course this is just a vote about how misleading the julia function is, not that we should do something else.

lgoettgens avatar Mar 10 '25 20:03 lgoettgens

We had a discussion related to this during yesterday's triage and basically I now think that while "formally and legally" my new methods are "correct", they are footguns waiting to be triggered, so I now tend to agree that if we add ones it should be a modified duplicate of the existing zeros method, i.e. without aliasing.

fingolfin avatar May 15 '25 11:05 fingolfin