[Octave-patch-tracker] [patch #10288] Implementation of Matlab function tensorprod

[Nicholas Jankowski]

Update of patch #10288 (project octave):

                  Status:             In Progress => Ready For Test         

    _______________________________________________________

Follow-up 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).




    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/patch/?10288>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/