patchGNU Octave - Patches: patch #10288, Implementation of Matlab function...

 
 

patch #10288: Implementation of Matlab function tensorprod

Submitter:  Kasper H. Filtenborg <filtenborg>
Submitted:  Thu 20 Oct 2022 10:40:30 PM UTC
   
 
Category:  Core : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 08 Mar 2023 05:05:57 PM UTC, comment #24: 

Okay.  I removed support for NumDimensionsA=VALUE syntax until there is Octave-wide support for this mechanism.  See http://hg.savannah.gnu.org/hgweb/octave/rev/8c7dcaf2e1b6.

Rik <rik5>
Group administrator
Wed 08 Mar 2023 02:12:18 AM UTC, comment #23: 

from jwe's earlier comment after Kasper had implemented a different way of faking that option syntax:

comment #2:

> Until we have fully implemented the new arguments block syntax in Octave I don't think we should try to fake support for the keyword=value option syntax.
>
> If you want to handle optional parameters, you can correctly support "keyword", value pairs using inputParser or other means, or you can accept option structs.
>


is that something we want to do?

Nicholas Jankowski <nrjank>
Group Member
Wed 08 Mar 2023 01:14:29 AM UTC, comment #22: 

I reviewed tensorprod.m and made some changes for performance.  I also implemented the feature to accept 'NumDimensionA=XXX' which was listed as a FIXME.  See http://hg.savannah.gnu.org/hgweb/octave/rev/708741d6d29b.

Marking this patch as done and closing report.

Rik <rik5>
Group administrator
Thu 01 Dec 2022 12:05:33 AM UTC, comment #21: 

- for future reference and to help our google SEO:  https://wiki.octave.org/Help_text_style_guide

- sorry, I didn't think to manually add your name to the contrib list. i thought we manually scrape those from commit messages, but maybe we stopped that.

added that and the 'two spaces' tensorprod doc update to
https://hg.savannah.gnu.org/hgweb/octave/rev/53fee053d962

(and now I see i botched the commit message when i backed out and reapplied it as a patch. @jwe I'm assuming we don't hg ci --amend those out somehow and I just live with my shame?)

regarding the " or '  ... I've always tried to do "" strings as Octave has generally pushed, so defaulted to ' inside for nested quotes, but obviously our consistency is not 100%.  I've worked on griddata, it's entirely possible I wrote that error message with the alternate form ¯\_(ツ)_/¯.

Nicholas Jankowski <nrjank>
Group Member
Wed 30 Nov 2022 10:05:53 PM UTC, comment #20: 

1 - I was not aware of the help text style guide - I have found it now - my bad. (I have been attempting to follow https://wiki.octave.org/Octave_style_guide. Here, double quotes in a text string are escaped as "This \"string\" contains a\nnewline" except when it "becomes inconvinient" to do so. It also mentions max. 79 characters per line instead of max. 80.)

2 - Not to enter a long discussion as I am happy to accept either way, but a counter example is the function "griddata" where the error message 'griddata: "%s" interpolation not yet implemented' can be found. Same applies at least for "ode23".

3 - Thanks - got it.

4 - As I wrote initially, if I am allowed in the list of contributors, it would be an honour. I didn't want to sneak that into the patch :)

Kasper H. Filtenborg <filtenborg>
Wed 30 Nov 2022 01:54:13 AM UTC, comment #19: 

1 - Thanks. there is actually a help text style guide.  we're supposed to double space after periods, no mention of other punctuation, but I'd assume 2 after the : is consistent. I should probably go back and fix the rest of the docstring for that.

2 - if you look at the help for var, you'll see error messages regarding the 'all' option that use the "error: message about 'all'" format. This is consistent with Octave's general use of double quoted strings, making it easier to put single quotes inside  to be echoed with the string.

3 - Octave's mercurial source code system has two primary branches - stable and default.  Stable is 'production code', being readied for the next release, and generally only directly getting bugfixes, documentation fixes, etc. default is where most 'hacking' and feature introduction should take place. Official releases are made from stable (e.g., the recent 7.3.0), and for a major release, at some point there's a feature freeze on default, it's merged into stable, and then prepared for release (e.g., the upcoming 8.1), and then the default branch becomes the location for the next major release (9).  We could create new branches for different features, but generally don't, instead using submitted patches to capture code progress.

Nicholas Jankowski <nrjank>
Group Member
Tue 29 Nov 2022 11:06:38 PM UTC, comment #18: 

Thank you for pointing out the changes that you made. They make a lot of sense, and they will be good to have as a reference should I have another contribution.

I have looked through the file and have not spotted any problems. I have only two really minor things:
 - Double space in line 53 (after the colon)
 - Shouldn't the use of single and double quotes be flipped in the error messages where both are used so that e.g. '"all"' is used instead of "'all'" to produce "all" in the printed error message? I think this is more consistent with other printed information as e.g. "help tensorprod". (I see that some functions are using 'foo' and others "foo" in error messages.)

And just for my understanding, how does the compiler know not to include the changes from the patch? Shouldn't there be another branch for version 9?

Kasper H. Filtenborg <filtenborg>
Fri 25 Nov 2022 07:10:51 PM UTC, comment #17: 

ok, i made some minor changes. 

- It's fine using print usage for the simple too few/too many input count check. 
- Added a few other input validation BISTs.
- Pulled comments inside of 80 chars (yes often docstring fn defs and sometimes tests are allowed to run over. left those alone.)
- there's no hard rule on indenting continued message strings. generally some people like to push it over to the start of the argument list block being split. You can see where I did this is for most of your error messages. We generally prefer spacing between operators, so the spacing before the ... as you did is fine.
- errors and warnings generally use single quotes to offset variable/option names and reduce the need for escape characters.

- your main block of unit tests verify that the code runs without error, but that's it. The values coming out could be completely erroneous, or the wrong shape, etc., and that wouldn't set off most of the tests. I've created tests with simple values for scalars, vectors, matrices, and tensors that I think cover the same cases as your random input tests, and added another block for NumDimensionsA tests.  Also, note that i tried to use shared variables and separate tests instead of one long test block. That way if one test fails it'll be easier to locate.  Currently all tests pass. 

Let me know if you see anything significant missing. There may also be some optimizations that would make sense for this function - example, shortcuts if either input is a scalar, or null, or the identity matrix, etc. But I think those can get pushed later as separate patches.

We're currently in a feature freeze for v8, so I refreshed the patch, updated the commit message, and pushed it under your name for v9. If someone things this should sneak in for 8, i can back out the change and push it to 8 instead.
https://hg.savannah.gnu.org/hgweb/octave/rev/212cc32100f5

marking as Ready for Test.

(And thanks for pointing out the issue with mean.  bug #63410 has now addressed that, and we're looking at any other regressions since it was significantly overhauled for 8.1, which didn't catch Inf, Nan, or neg dims).


Nicholas Jankowski <nrjank>
Group Member
Thu 24 Nov 2022 03:54:31 AM UTC, comment #16: 

Ok. Had started looking through the other function and there were few changes I was going to recommend. Let me check out the patch version and I’ll get back in a bit with those.

Nicholas Jankowski <nrjank>
Group Member
Thu 24 Nov 2022 12:16:14 AM UTC, comment #15: 

Thanks for the swift response.

I have setup so that I can create patches. Looking to the patch file in patch #10283, I think that I have found the relevant files to change. In addition to the tensorprod.m file, the patch includes:
- Add the tensorprod.m in the relevant module.mk file
- Add the docstring to the documentation for the linear algebra module
- tensorprod is referenced under "see also" in dot, kron, and mtimes

The function itself is the same as the previous file (file #53916) with the changes:
- Copyright notice is changed to match Octave standard
- "mod" replaced by "rem" as this seems to be slightly faster (I saw the update for mean.m mentioned in bug #63410 and got inspired to test which was faster)

Please note that I have not build and tested Octave with this patch applied.

(file #54000)

Kasper H. Filtenborg <filtenborg>
Wed 23 Nov 2022 04:23:10 PM UTC, comment #14: 

sorry let this sit.  will take a look at it. It's a fairly isolated function add without much impact elsewhere. It might be able to be added for v8.1, but will await other consensus for that. in either case are you set up to be able to put a patch together to add the function plus other necessary changes to the octave source?


Nicholas Jankowski <nrjank>
Group Member
Tue 22 Nov 2022 09:49:40 PM UTC, comment #13: 

I have not heard back after the last update to the function where I hopefully accommodated all of your remaining requests. I guess you have been busy with the version 7.3 release and now with the upcoming version 8.

Is there anything further I can do to support the release of the tensorprod function? Perhaps there is a chance to have it accepted for version 8?

Kasper H. Filtenborg <filtenborg>
Thu 27 Oct 2022 11:59:03 PM UTC, comment #12: 

Thank you all for your valuable comments. I have changed the following:

- Accept only single and double for A and B
- Improved description of error messages (removed all use of print_usage() except tensorprod())
- Reports errors for combinations of invalid inputs that were not considered before and added checks for these in the self test
- Where possible, cleared up sections with error checking so that values are assigned after the checks are done instead of in the middle of a section with checks
- Long lines are split to keep max. 79 characters (I don't know how I missed that). Looking at other files, the function descriptions and the self tests seem to violate the max. 79 characters.
  --> Is this ok?
- After a line has been split, I have put an additional indentation (two spaces) to indicate that the line is a continuation of the previous line. Additionally, before the "..." I have put a single space.
  --> What is the correct way to do this?
- Added support for row vectors for the dimensions. A combination of row and column vector for dimA and dimB is also valid.
- Changed to size_equal(A,B) as suggested
- a==fix(a) changed to mod(a,1)==0, which is slightly better since it can handle Inf. I am not trying to invent something new with the various checks. Often they are inspired by other checks I have found by viewing other functions. E.g. the a==fix(a) comes directly from mean.m, and here mean([1,2,3],Inf) fails without a good descriptive error as e.g. mean([1,2,3],NaN) does.

(file #53916)

Kasper H. Filtenborg <filtenborg>
Mon 24 Oct 2022 05:50:33 PM UTC, comment #11: 

thanks. size_equal also apparently ~2x as fast as the any() method.

Nicholas Jankowski <nrjank>
Group Member
Mon 24 Oct 2022 05:10:16 PM UTC, comment #10: 

`any(size(A)!=size(B))` might fail with an unexpected error if the number of dimensions of `A` and `B` don't match. Consider using `size_equal (A, B)` instead.

Markus Mützel <mmuetzel>
Group administrator
Mon 24 Oct 2022 03:58:32 PM UTC, comment #9: 

It's probably best to limit the inputs to be floating point.

For example, what happens for things like


a = int64 (flintmax) + int64 (1);
b = int64 (5);
assert (a * b, tensorprod (a, b))


?  It seems bad to silently return incorrect/inaccurate results when simple multiplication can return an exact result.

John W. Eaton <jwe>
Group administrator
Mon 24 Oct 2022 03:47:34 PM UTC, comment #8: 

running commentary:

try to limit line lengths to 80 characters. long strings can be split/concatenated across multiple lines using [,], long lines can be split with ...

i think if !isequal(size(A),size(B)) could be replaced with if any(size(A)!=size(B)) to avoid the more expensive call to isequal.

your dim checks aren't actually checking if the inputs are integer valued. i think a==fix(a) is a fairly low overhead check for this but others might know a better method.

try to group similar tests.  e.g., the 'Invalid call to' tests are usually grouped at the top, followed by individual error message testing. And it's not a hard rule, but a specific error message is usually more informative to the reader than print_usage, as it doesn't point out the actual error. We often limit it to the 'incorrect number of inputs' type errors. e.g., in the ischar check followed by the 'is it 'all'' check, it would be better to report back something like error("unknown parameter %s", varargin{1}) to actually point them to the specific error.  if it fails the isnumeric and ischar tests, an "A must be of type...." error would be more informative than print_usage.

in the initial error messages, I'd probably use the phrase "A must be a numeric array" vs tensor, as 'array' is the name of that type of numerical element.

i can try a series of empty and nan input shape tests later to verify matlab outputs, in addition to confirming your other tests.

 



 

Nicholas Jankowski <nrjank>
Group Member
Mon 24 Oct 2022 02:32:42 PM UTC, comment #7: 

in matlab, A and B are restricted to singles and doubles.  isnumeric allows ints to pass as well. is that behavior desired? (I originally thought it would be best to use isfloat() to check that, but now I can't of a case where two ints would cause problems. Adding and multiplying should still result in an int at output.  What about if the types are mixed? would we want it to convert to double and output the actual result, or preserve the int class and at some point along the way truncate the results?

since this would be a superset of what matlab does, there's no immediate compatibility concern with the choice.  In the interest of avoiding any output ambiguity, I would suggest issuing a warning and promoting the int to (double or single) if A and B are not both int, then carrying on with the normal calculations. (assuming we don't want to just limit to type float)

Nicholas Jankowski <nrjank>
Group Member
Mon 24 Oct 2022 12:41:25 AM UTC, comment #6: 

In the self test, the error catching part was not setup correctly.

Here it is corrected.

(file #53898)

Kasper H. Filtenborg <filtenborg>
Mon 24 Oct 2022 12:21:24 AM UTC, comment #5: 

Thanks for the comments. I think that you have some very valid points.

I have added further validation of inputs and attempted to add a self test to the function. This is not something that I have a lot of experience with.

For the empty inputs, I have used the demo in the Matlab help (the Try This Example button) to see the size of the output and the output itself using various empty arrays as inputs. From what I can see, I get the same dimensions as the Matlab function.

Another update is attached.

(file #53897)

Kasper H. Filtenborg <filtenborg>
Sun 23 Oct 2022 06:32:34 AM UTC, comment #4: 

Ok, I had wondered if that form would work. thanks for checking.  It would probably be worth adding an error that catches when people try to call it with NumDimensionsA=4 form, the message indicating something like"name=value syntax arguments are not yet implemented in Octave".

Regarding the rest of the code the actual tensor product portion seems mathematically correct. There are a lot of spots where error handling could be improved, however, especially for input type-checking. These spots would probably be easy to identify if you could add self tests to the end of the function  (see https://wiki.octave.org/Tests, and you can look at most other octave functions for some examples.) In particular:

some basic correct outputs should be tested in all of the different expected calling forms.  (I got a 'dimA not defined" error with tensorprod(a,b,2) ).  For compatibility, that usually should also include empty and NaN inputs of different shapes, which could involve requesting a list of outputs from someone with access to a recent version of matlab.

anything expected to be an error should have an error test in an Input Validation section.  If we expect something to produce an error, the function should have an explicit check and sensible error message to explain it to the user. Some in particular:

- calling with nargin > 6 (you can add a "|| nargin > 6" to you print_usage check)
- calling with name=value form.
- A and B not being single or double (isfloat() can check that)
- typechecking any varargin for not being an integer, "all", or "NumDimensionsA".  e.g., checking for tensorprod(a,b,"foo")

 

Nicholas Jankowski <nrjank>
Group Member
Sun 23 Oct 2022 12:30:13 AM UTC, comment #3: 

Thank you for your replies. It feels like that I have come to the right place.

Reading your replies clears up a lot. Previously, I had never seen the new argument syntax used in Matlab nor Octave and I assumed that the property-value pair method would not work. Since I could find no other implementation and I sought to make the function Matlab compatible, I looked for various options and came up with the idea to use the inputname function with the "ids_only" option set to false. I now see that the Matlab tensorprod function in fact is compatible with the property-value pair (e.g. tensorprod(..., "NumDimensionsA", 4)) even though, it is not explicitly stated in the Matlab help.

I have changed the function to accept the property "NumDimensionsA" (together with a value) and discarded the other implementation. While at it, I also made some minor changes to checks, error messages, comments, etc. - nothing about the actual computation is changed.

The updated file is attached.

(file #53893)

Kasper H. Filtenborg <filtenborg>
Sat 22 Oct 2022 03:26:53 AM UTC, comment #2: 

Until we have fully implemented the new arguments block syntax in Octave I don't think we should try to fake support for the keyword=value option syntax.

If you want to handle optional parameters, you can correctly support "keyword", value pairs using inputParser or other means, or you can accept option structs.

John W. Eaton <jwe>
Group administrator
Fri 21 Oct 2022 08:02:21 PM UTC, comment #1: 

thanks for the contribution!  i'm looking it over a bit now. 

this is the first time I've seen an Octave implementaiton of the option form used by tensorprod(...,NumDimensionsA=ndimsA). I do see it's behaving a bit differently outside the scope of the function in Octave vs matlab.  using the example in the matlab help:


A = rand(3,4);
B = rand(3,4,5);
C1 = tensorprod(A,B,1);
C2 = tensorprod(A,B,1,NumDimensionsA=3);


in Octave:

>> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name                Size                     Bytes  Class
  ====   ====                ====                     =====  =====
         A                   3x4                         96  double
         B                   3x4x5                      480  double
         C1                  4x4x5                      640  double
         C2                  4x1x4x5                    640  double
         NumDimensionsA      1x1                          8  double
         ans                 1x4                         32  double

Total is 237 elements using 1896 bytes


in Matlab 2022b:


>> whos
  Name      Size               Bytes  Class     Attributes

  A         3x4                   96  double
  B         3x4x5                480  double
  C1        4x4x5                640  double
  C2        4x1x4x5              640  double
  ans       1x4                   32  double


I think there's been some discussion on how to handle this option calling form over at octave.discourse.group, but I don't think there's anything to be done within the scope of you implementing this function. it looks like the strcmp(strtok(inputname...)) approach does the job well enough.

will get back to you after I've had a chance to look it over and run some matlab checks against it.

Nicholas Jankowski <nrjank>
Group Member
Thu 20 Oct 2022 10:40:30 PM UTC, original submission:  

I have implemented the Matlab function tensorprod. The implementation is inspired by tensordot from numpy (https://github.com/numpy/numpy/blob/main/numpy/core/numeric.py), but is changed significantly to match the expected behaviour according to the Matlab documentation (https://se.mathworks.com/help/matlab/ref/tensorprod.html) as well as to make better use of functions in Octave.

The function has not been compared directly against Matlab since I do not have access to Matlab.

Kind regards,
Kasper H. Filtenborg

PS If you find this contribution worthwhile, it would be an honour to be listed as a contributor in the Octave documentation.

Kasper H. Filtenborg <filtenborg>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #54000:  tensorprod.patch added by filtenborg (18KiB - application/octet-stream)
file #53916:  tensorprod.m added by filtenborg (15KiB - application/octet-stream - Revision 5)
file #53898:  tensorprod.m added by filtenborg (12KiB - application/octet-stream - Revision 4)
file #53897:  tensorprod.m added by filtenborg (12KiB - application/octet-stream - Revision 3)
file #53893:  tensorprod.m added by filtenborg (6KiB - application/octet-stream - Revision 2)
file #53889:  tensorprod.m added by filtenborg (5KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by filtenborg (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-03-08 rik5 StatusReady For Test Done
        Open/ClosedOpen Closed
    2022-11-25 nrjank StatusIn Progress Ready For Test
    2022-11-24 filtenborg Attached File- Added tensorprod.patch, #54000
    2022-10-27 filtenborg Attached File- Added tensorprod.m, #53916
    2022-10-24 filtenborg Attached File- Added tensorprod.m, #53898
    2022-10-24 filtenborg Attached File- Added tensorprod.m, #53897
    2022-10-23 filtenborg Attached File- Added tensorprod.m, #53893
    2022-10-21 nrjank StatusNone In Progress
    2022-10-21 siko1056 PrivacyPrivate Public
    2022-10-20 filtenborg Attached File- Added tensorprod.m, #53889

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code