changed the definition of Randomize
#3619
Conversation
As proposed in `lib/matobj2.gd`: For a random source `rs`, document `Randomize( rs, obj )` instead of `Randomize( obj, rs )`, and then use `InstallMethodWithRandomSource` to install methods. Note that after these changes, results of `Randomize` from older GAP sessions may not be reproducible because former calls of the form `Random( C )` are now executed as `Random( GlobalMersenneTwister, C )`. - added an `<Oper Name="Random" .../>` declaration for random source and collection, to be used in cross-references; note that the `DeclareOperation` calls had covered this already. - added `Randomize` to the documented operations for which the function `InstallMethodWithRandomSource` is intended - used `InstallMethodWithRandomSource` in order to replace two method installations by just one (as was proposed in `matobj2.gd`) for all installations of `Randomize` methods in the GAP library - make sure that all `Randomize` methods return the changed object - adjusted outputs of the testfile `testinstall/MatrixObj/Randomize.tst` (one instance of the changed outputs mentioned above)
ThomasBreuer
added
kind: enhancement
Codecov Report
@@ Coverage Diff @@
## master #3619 +/- ##
===========================================
- Coverage 84.64% 72.27% -12.37%
===========================================
Files 698 635 -63
Lines 345549 308785 -36764
===========================================
- Hits 292482 223173 -69309
- Misses 53067 85612 +32545
|
ThomasBreuer
force-pushed
the
TB_Randomize
branch
from
c48dcf3
to
a6fddb8
Compare
| InstallMethodWithRandomSource( Randomize, | ||
| "for a random source and a mutable gf2 vector", | ||
| [ IsRandomSource, IsGF2VectorRep and IsMutable ], | ||
| function( rs, v ) | ||
| local i; | ||
| MultVector(v,0); | ||
| for i in [1..Length(v)] do | ||
| if Random(rs,0,1) = 1 then v[i] := Z(2); fi; |
There was a problem hiding this comment.
Not relevant for this PR, but: this is something which probably would be several orders of magnitudes faster if done in C, as we could just generate random data in larger chunks (e.g. 64 bits at a time) and initialize the memory with it as needed, instead of iterating over each bit.
| vec[ i ] := Random( rs, basedomain ); | ||
| InstallMethodWithRandomSource( Randomize, | ||
| "for a random source and a matrix object", | ||
| [ IsRandomSource, IsMatrixObj and IsMutable ], |
There was a problem hiding this comment.
The code this replaces was using IsVectorObj, and now here is IsMatrixObj? Am I missing something? Aha, perhaps this is simply a new function that was not present before, and the diff is therefore misleading? In that case, it would be nice if the commit message / PR description explicitly mentioned this new functionality (but it's not super important either, it's just a suggestion).
lib/matobj.gi
Outdated
| basedomain := BaseDomain( mat ); | ||
| for i in [ 1 .. NrRows( mat ) ] do | ||
| for j in [ 1 .. NrCols( mat ) ] do | ||
| mat[i][j]:= Random( rs, basedomain ); |
There was a problem hiding this comment.
| mat[i][j]:= Random( rs, basedomain ); | |
| mat[i,j]:= Random( rs, basedomain ); |
| InstallOtherMethod( Randomize, | ||
| "backwards compatibility: swap arguments", | ||
| [ IsObject and IsMutable, IsRandomSource ], | ||
| { obj, rs } -> Randomize( rs, obj ) ); |
There was a problem hiding this comment.
This compatibility method ensures that existing code using Randomize will keep working, and can thus be adapted incrementally -- very good. However, existing implementations will need to be adapted, else they won't get called... I don't see a good alternative to this, anyway (other than installing another method which swaps the arguments in another order, and then perhaps also a horrible hack which prevents us from doing an infinite recursion in case no specialized method is provided). Luckily, only three packages provide such methods: cvec and recog (which I'll both adapt now, and make releases), and also meataxe64, but that's not yet deposited and I am sure @stevelinton will be happy to adapt it, resp. we can provide a PR. (And by adapting, I mean I'll install methods for both argument orders, this way the packages will work with both old and new GAP versions).
| ## Replaces every entry in the mutable vector object <A>v</A> | ||
| ## or matrix object <A>m</A>, respectively, with | ||
| ## a random one from the base domain of <A>v</A> or <A>m</A>, | ||
| ## respectively, and returns the argument. |
There was a problem hiding this comment.
Note that the documentation in orb says that nothing is returned. I prefer your variant, though, it is useful in some situations. However, this is one more thing that needs to be adapted in recog, cvec and meataxe64. (I'll look into doing that right now).
and fixed some `[i][j]` occurrences
ThomasBreuer
force-pushed
the
TB_Randomize
branch
from
a6fddb8
to
db97a1f
Compare
dimpase
added
release notes: added
As proposed in
lib/matobj2.gd:For a random source
rs,document
Randomize( rs, obj )instead ofRandomize( obj, rs ),and then use
InstallMethodWithRandomSourceto install methods.The release notes should mention this change mainly because
Randomizeis used in a few packages.Another aspect is that after the change, results of
Randomizefrom older GAP sessions may not be reproducible because former calls of the formRandom( C )are now executed asRandom( GlobalMersenneTwister, C ).Details:
added an
<Oper Name="Random" .../>declaration forrandom source and collection, to be used in cross-references;
note that the
DeclareOperationcalls had covered this already.added
Randomizeto the documented operations for whichthe function
InstallMethodWithRandomSourceis intendedused
InstallMethodWithRandomSourcein order to replacetwo method installations by just one (as was proposed in
matobj2.gd) for all installations ofRandomizemethodsin the GAP library
make sure that all
Randomizemethods return the changed objectadjusted outputs of the testfile
testinstall/MatrixObj/Randomize.tst(one instance of the changed outputs mentioned above)
Resolves #2018