MatrixObj: MultRowVector #1685

ssiccha · 2017-09-07T11:07:05Z

Adjust some test files.
Get rid of IsCheckingVector for IsPlistVectorRep and same for matrices
Add some GAPDoc documentation
Implementation and Tests for MultRowVector

codecov · 2017-09-07T12:06:17Z

Codecov Report

Merging #1685 into MatrixObj2 will increase coverage by <.01%.
The diff coverage is 48.1%.

@@              Coverage Diff               @@
##           MatrixObj2    #1685      +/-   ##
==============================================
+ Coverage       64.46%   64.46%   +<.01%     
==============================================
  Files            1002     1002              
  Lines          326543   326564      +21     
  Branches        13218    13220       +2     
==============================================
+ Hits           210505   210528      +23     
+ Misses         113167   113162       -5     
- Partials         2871     2874       +3

Impacted Files	Coverage Δ
lib/obsolete.gd	`28.35% <ø> (ø)`	⬆️
lib/listcoef.gi	`52.91% <ø> (+0.08%)`	⬆️
lib/vec8bit.gi	`48.14% <ø> (ø)`	⬆️
hpcgap/lib/vec8bit.gi	`46.87% <ø> (ø)`	⬆️
hpcgap/lib/vecmat.gi	`65.28% <0%> (ø)`	⬆️
lib/obsolete.gi	`0% <0%> (ø)`	⬆️
lib/matobj.gi	`8.67% <0%> (-0.46%)`	⬇️
lib/matobjplist.gi	`0% <0%> (ø)`	⬆️
lib/grpffmat.gi	`91.34% <0%> (ø)`	⬆️
lib/vecmat.gi	`67.32% <0%> (ø)`	⬆️
... and 27 more

frankluebeck · 2017-09-07T12:58:26Z

We want to remove commands XXXRowVectorYYY, just use Vector instead of RowVector.
Furthermore, for general semirings as coefficients, we need to distinguish scalar multiplication from the left and from the right.
E.g.,
- introduce and document MultVectorLeft(vec, c) and MultVectorRight(vec, c) with the obvious meaning;
- define and document MultVector as synonym for MultVectorLeft;
- for backward compatibility define MultRowVector as further synonym for MultVectorLeft (either silently without documentation, or document this in an "obsolete functions" section).
Similarly, introduce AddVectorLeft, AddVectorRight and synonyms AddVector and for compatibility AddRowVector.

fingolfin · 2017-09-09T22:27:23Z

lib/listcoef.gd

 ##
 ##  <#GAPDoc Label="MultRowVector">
+##  <#GAPDoc Label="MultRowVectorLeft">
+##  <#GAPDoc Label="MultRowVectorRight">


Why are these called MultRowVectorLeft/Right? In the new scheme, they should be MultVectorLeft/Right, no?

Last week, you mentioned wanting to rename them in a second step later on, but I don't understand why -- why would we introduce a new name, only to then rename it immediately after?

Also note that the above is invalid syntax, and cause make doc to fail.

fingolfin · 2017-09-09T22:30:20Z

lib/listcoef.gd

+##  <C>MultRowVectorLeft</C>.
+##  </P>
+##  These operations multiply <A>list1</A> with <A>mul</A> in-place and write
+##  the result to <A>list1</A>.


"... [modify] list1 ... in-place and write the result to list1" seems a bit redundant... I'd stop after "in-place". Or, if you really want to elaborate on that, perhaps: "... in-place, i.e., the result overwrites list1" or so -- here, the "i.e." makes clear the following is just a clarification of what came before.

fingolfin · 2017-09-09T22:35:59Z

lib/listcoef.gd

 ##  <ManSection>
 ##  <Oper Name="MultRowVector" Arg='list1, [poss1, list2, poss2, ]mul'/>
+##  <Oper Name="MultRowVectorLeft" Arg='list1, [poss1, list2, poss2, ]mul'/>
+##  <Oper Name="MultRowVectorRight" Arg='list1, [poss1, list2, poss2, ]mul'/>


You write later on that the 5-arg version will be removed. So, why document it here?

OK, I could understand if you want to document it for MultRowVector, for the sake of people who may have used the 5-arg version and want to find out what happened / is going to happen to it.

But in that case, I'd rather split this documentation: Write one mansection which just discusses MultVectorLeft/Right. In it, only list and document the 2-arg versions.

Then, have a separate man section for MultRowVector, which just says that its 2-arg version is just a synonym of MultVectorLeft, and that the 5-arg version is obsolete, should not be used, and is only there for backwards compatibility.

Bonus points for moving MultRowVector into lib/obsolete.{gi,gd}.

fingolfin · 2017-09-09T22:36:07Z

lib/listcoef.gd

 ##  <Description>
-##  The five argument version of this operation replaces
+##  Note that <C>MultRowVector</C> is only a Synonym for
+##  <C>MultRowVectorLeft</C>.


"only" -> "just", "Synonym" -> "synonym"

It also seems weird to start with this side remark. I'd move this sentence to a later spot, say right after MultRowVectorLeft has been declared.

fingolfin · 2017-09-09T22:36:26Z

lib/listcoef.gd

+##  <C>MultRowVectorLeft</C> multiplies <A>mul</A> to <A>list1</A> from the
+##  left. <C>MultRowVectorRight</C> does so from the right.
+##  The two-argument version simply multiplies each element of <A>list1</A>,
+##  in-place, by <A>mul</A> from the left.


I was about to write "simply", but upon closer inspection, I'll rather say: Just drop that sentence, it is redundant.

fingolfin · 2017-09-09T22:39:31Z

lib/listcoef.gi

+#M  MultRowVectorLeft( <list1>, <poss1>, <list2>, <poss2>, <mult> )
 ##
-InstallMethod( MultRowVector,"generic method",
+InstallMethod( MultRowVectorLeft, "generic method",


This should be (in my proposed new scheme):

InstallOtherMethod( MultRowVector, "obsolete five argument method",

fingolfin · 2017-09-09T22:39:56Z

lib/listcoef.gi

 end );

-InstallOtherMethod( MultRowVector,"error if immutable",true,
+InstallOtherMethod( MultRowVectorLeft, "error if immutable", true,


revert this line back

fingolfin · 2017-09-09T22:40:59Z

lib/listcoef.gi

-InstallOtherMethod( MultRowVector,
-        "Two argument kernel method for small list",
+InstallMethod( MultRowVectorLeft,
+    "Two argument kernel method for small list",


"Two" -> "two"

But actually, more idiomatic would be to write "for a mutable dense small list and a multiplicative element" -- and this of course also applies to other InstallMethod calls you are adding or modifying.

fingolfin · 2017-09-09T22:44:02Z

lib/listcoef.gi

-        [ IsSmallList and IsDenseList and IsMutable and IsPlistRep and
-          IsCyclotomicCollection,
+    [ IsSmallList and IsDenseList and IsMutable and IsPlistRep and
+      IsCyclotomicCollection,


IsPlistRep implies IsSmallList so you can get rid of that.

fingolfin · 2017-09-09T22:48:16Z

lib/matobj2.gd

 ############################################################################


 # The following are guaranteed to be always set or cheaply calculable:


Remove that line? Or at least put an empty line between it and the new comment below.

fingolfin · 2017-09-09T22:59:46Z

This PR also mixes several things together: namely you remove IsCheckingMatrix etc., and deal with Mult(Row)Vector. This way, reviewers have to always look at both, and issues with one hold up the merge of the other.
I therefore recommend splitting this PR into two: Leave the MultRowVector stuff on this PR, but remove the IsCheckingMatrix changes from it into a new PR (git rebase -i can help with that; if you are not familiar with it, just ask on slack).

When doing that, you can also squash the first commit ("typos") with the one that removes IsCheckingMatrix (both "typos" are in functions which are removed later on anyway).

Finally, the commit "MatrixObj: Remove test diffs due to ws/line breaks" could just be pushed directly to MatrixObj2

ssiccha · 2017-09-12T18:34:52Z

Thanks for the review! I just split some commits into different PRs as you suggested. I'll try to get to the requested changes and clean up this PR tomorrow.

ssiccha · 2017-09-13T12:03:19Z

Ah, I'll rename the *RowVector -> *Vector too

ssiccha · 2017-09-13T13:47:55Z

This passes make testinstall if PR #1718 gets merged. I'll have to somehow rerun the tests then.

ssiccha · 2017-09-13T13:51:44Z

I additionally changed:

Make MultVectorLeft a synonym for MultVector. Then people who are only interested in commutative rings shouldn't get any MultVectorLeft things anywhere.
Have a kernel function MULT_ROW_VECTOR_LEFT_RIGHT_2 which is called by
MULT_ROW_VECTOR_LEFT_2 and ..RIGHT_2
Only do the ..Left version in listcoef.g?. The methods from listcoef.gi are only used for integers and FFEs in the library.
Adding the ..Right versions would only introduce lots of unused code, I think. People wanting to use the ..Right versions then should use vector objs.

fingolfin · 2017-09-14T11:14:14Z

lib/matobj2.gd

-
-# maybe have this:   vec := vec{[from..to]} * scal ?? cvec has it
+# TODO: rename MultVector to MultVector; but keep in mind that
+# historically there already was MultVector, so be careful to not break that


That TODO is outdated (and now impossible to understand)

fingolfin · 2017-09-14T11:16:12Z

lib/listcoef.gd

-##  The two-argument version simply multiplies each element of <A>list1</A>, 
-##  in-place, by <A>mul</A>.
+##  Note that <C>MultVectorLeft</C> is just a synonym for
+##  <C>MultVector</C>.


Hmmm... perhaps it should be the other way around? What's the motivation for doing it this way?

In the end, it's not that important, of course; but for reasons of symmetry, I'd find it more natural if the left/right names were the "regular" ones, and the one without a convenience alias.

fingolfin · 2017-09-14T11:17:11Z

lib/listcoef.gi

+#M  MultVectorLeft( <list>, <mul> )
 ##
-InstallMethod( MultRowVector,"generic method",
+InstallMethod( MultVectorLeft,


If MultVectorLeft is just a synonym, it would be more natural to install the method for MultVector here...

fingolfin · 2017-09-14T11:18:10Z

lib/listcoef.gi

-      IsDenseList,
-      IsDenseList,
      IsMultiplicativeElement ],
    0,


While you are at it, you might just as well remove the true and 0 here, as they are superfluous.

Same for the other InstallMethod calls you change

fingolfin · 2017-09-14T11:19:36Z

lib/matobj2.gd

+##  <Returns>nothing</Returns>
+##
+##  <Description>
+##  Note that <C>MultVector</C> is only a Synonym for


"Synonym" -> "synonym"

fingolfin · 2017-09-14T11:22:54Z

lib/matobj2.gd

+DeclareOperation( "MultVectorRight",
+  [ IsVectorObj and IsMutable, IsMultiplicativeElement, IsInt, IsInt ] );
+#  Note that MultVector is declared a Synonym for MultVectorLeft in
+#  listcoef.gd


fix "Synonym"

fingolfin · 2017-09-14T11:28:22Z

lib/obsolete.gd

+##  <Oper Name="MultRowVector" Arg='list1, [poss1, list2, poss2, ]mul'/>
+##  <Returns>nothing</Returns>
+##
+##  <Description>


This text should make it explicit that new code should not use MultRowVector anymore. So perhaps replace "is just a synonym" by "is an obsolete synonym", and add at the end "New code should use MultVectorLeft instead." or so

Moreover, this documentation section is (I think) still being included by doc/ref/vector.xml -- can you move it to doc/ref/obsolete.xml ?

fingolfin · 2017-09-14T11:30:33Z

src/listoper.c

+        prd = PROD(prd,mult);
+        ASS_LIST(list,i,prd);
+        CHANGED_BAG(list);
+      }


Do you have clang-format installed? If so, please consider using git clang-format on your commits to reformat the modified code.

But don't worry about this if you don't have clang-format.

fingolfin · 2017-09-14T11:31:19Z

tst/test-matobj/MultRowVector.tst

+gap> MultRowVector(v, -1);
+gap> Unpack(v);
+[ Z(5)^2, Z(5)^3, Z(5), Z(5), 0*Z(5), Z(5)^0 ]
+gap> MultRowVector(v, Z(5)^3, 3, 4);


Why does this still use MultRowVector ?

fingolfin · 2017-09-14T11:31:43Z

tst/test-matobj/MultRowVector.tst

+<algebra-with-one of dimension 4 over Rationals>
+gap> q := NewVector( IsPlistVectorRep, Q, [One(Q), Zero(Q), Q.2, Q.3] );
+<plist vector over AlgebraWithOne( Rationals, [ e, i, j, k ] ) of length 4>
+gap> MultRowVectorRight(q, Q.2);


Seems the test file was not updated to match the name change?

fingolfin · 2017-09-28T01:48:00Z

@ssiccha any plans tomupdate this?

ssiccha · 2017-10-07T07:17:17Z

@fingolfin yep, I'll try to update it until next week. The last weeks were pretty busy.

- Split MultRowVector to MultRowVectorLeft / MultRowVectorRight. - MultRowVector is declared a synonym for MultRowVectorLeft. - MultRowVectorOp is renamed to MultRowVectorLeftOp on the C level. - Add four argument declarations for MultRowVectorLeft( vec, mul, from, to ) and ..Right - Remove the five argument version of MultRowVector in MatrixObj. Deprecate it also in the library. This version was not used anywhere in the library or packages - Add generic and plist implementations of MultRowVector where necessary - Implement and declare ..Left and ..Right versions in matrixobj* - Only do the ..Left version in listcoef.g?. The methods from listcoef.gi are only used for integers and FFEs in the library. Adding the ..Right versions would only introduce lots of unused code. People wanting to use the ..Right versions should use vector objs. - Have a kernel function MULT_ROW_VECTOR_LEFT_RIGHT_2 which is called by MULT_ROW_VECTOR_LEFT_2 and ..RIGHT_2 - Add tests for MultRowVector

ssiccha · 2017-10-07T19:38:45Z

@fingolfin I think it should be done now. All changes are in the "WIP" commit because I think that makes it easier to review. I will squash it into the previous commit later.

ssiccha · 2017-10-09T19:51:50Z

Oops, I forgot the hpcgap directory. Will update that hopefully tomorrow

ssiccha · 2017-10-19T17:05:42Z

@fingolfin OK, I hope it should be done now. I should probably have a look at which lines are covered by the tests.

Performs the following sed commands in the directories `lib/`, `src/`, `hpcgap/`. Then undo the changes in the `obsolete.{gd,gi}` files. sed -i 's/MULT_ROW_VECTOR/MULT_VECTOR/g' `find . -type f` sed -i 's/MULT_ROWVECTOR/MULT_VECTOR/g' `find . -type f` sed -i 's/MultRowVector/MultVector/g' `find . -type f` git checkout -- obsolete.{gd,gi} Also perform: sed -i 's/MultRowVector/MultVector/g' doc/ref/vector.xml

ssiccha changed the title ~~MatrixObj2~~ Some documentation and implementations for MatrixObj2 Sep 7, 2017

ssiccha force-pushed the MatrixObj2 branch 8 times, most recently from 0854fd9 to 9621032 Compare September 8, 2017 12:19

olexandr-konovalov added the gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall label Sep 9, 2017

fingolfin requested changes Sep 9, 2017

View reviewed changes

ssiccha force-pushed the MatrixObj2 branch 2 times, most recently from 15444db to dbc9466 Compare September 13, 2017 11:50

ssiccha force-pushed the MatrixObj2 branch from dbc9466 to 4027a97 Compare September 13, 2017 13:45

olexandr-konovalov mentioned this pull request Sep 13, 2017

Fix bug in DeclareObsoleteSynonym #1718

Closed

fingolfin requested changes Sep 14, 2017

View reviewed changes

ssiccha changed the title ~~Some documentation and implementations for MatrixObj2~~ MatrixObj: MultRowVector Sep 15, 2017

ssiccha force-pushed the MatrixObj2 branch from 4027a97 to 5f98ccb Compare September 15, 2017 14:15

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature labels Sep 28, 2017

fingolfin added this to the GAP 4.9.0 milestone Sep 28, 2017

ssiccha force-pushed the MatrixObj2 branch 3 times, most recently from 0f60c85 to f5a5bae Compare October 7, 2017 19:36

ssiccha force-pushed the MatrixObj2 branch from f5a5bae to 89049eb Compare October 19, 2017 16:56

ssiccha added 2 commits October 19, 2017 18:25

WIP: changes to MultRowVector PR

a7807bb

ssiccha force-pushed the MatrixObj2 branch from 89049eb to d654785 Compare October 19, 2017 18:26

fingolfin approved these changes Oct 24, 2017

View reviewed changes

fingolfin merged commit a0b1e68 into gap-system:MatrixObj2 Oct 24, 2017

fingolfin mentioned this pull request Aug 15, 2018

Rename MultRowVector to MultVector #2709

Merged

ssiccha deleted the MatrixObj2 branch March 14, 2020 10:50

		############################################################################


		# The following are guaranteed to be always set or cheaply calculable:

MatrixObj: MultRowVector #1685

MatrixObj: MultRowVector #1685

Uh oh!

Conversation

ssiccha commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

frankluebeck commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Sep 9, 2017

Uh oh!

ssiccha commented Sep 12, 2017

Uh oh!

ssiccha commented Sep 13, 2017

Uh oh!

ssiccha commented Sep 13, 2017

Uh oh!

ssiccha commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Sep 28, 2017

Uh oh!

ssiccha commented Oct 7, 2017

Uh oh!

ssiccha commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ssiccha commented Oct 9, 2017

Uh oh!

ssiccha commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ssiccha commented Sep 7, 2017 •

edited

Loading

codecov bot commented Sep 7, 2017 •

edited

Loading

ssiccha commented Oct 7, 2017 •

edited

Loading

ssiccha commented Oct 19, 2017 •

edited

Loading