(Note the following are not a critique of Paul's review, merely my opinions.) 0. I agree that a primary goal of the matrix package is to provide Matlab-like functionality. Our goal is not to *replicate* Matlab exactly, but we do want the same functionality. Wherever feasible and appropriate, we should make the package identical or as similar as possible in order to increase productivity and lower the learning curve. But the goal is not to make them identical -- the goal is to increase productivity and minimize the learning curve. 1. I like the natural index type (instead of positive). It may be one of my favorite things about the package. The existing VHDL integral array indices are all natural or integer besides STRING and fixed/float types. (Whose idea was the STRING range?!) Every other language I use as zero-based indexing. I write quite a bit of Matlab code, and it takes extra effort to remember the one-based indexing. And I mess it up regularly. And the math to calculate indices is messier. So it's harder to write, harder to debug, and the final product looks worse. Propagating this decision hurts productivity and the learning curve. Since this is VHDL, it would be nice if people can use 1:n index ranges if they like. The package should operate on them just the same as if they were indexed 0:n-1. (Does it already, David?) 2. I agree we should clarify "reversed." I agree that we should follow the VHDL convention of left-to-right processing (and top-to-bottom in this case). As with the previous point and SIGNED/UNSIGNED interpretation, I think your \mX+mY\ and \mX+mX\ should be identical. If I write out the matrix the same way it appears in a literal -- left-to-right, top-to-bottom -- then I can number the rows and columns however I like. But matrix addition, multiplication, etc. are defined in terms of the picture. Alternatively, we can disallow the "downto" direction similar to our discussion for the fixed-point package. 3. I liked the (start, length) inputs to the SubMatrix function because it side-stepped the index direction problem. 9,10. Agreed the zero power should be "eye", and the pow=2 case can be removed. 18. "dim" seems like the wrong name for the second argument of rot90. Perhaps "quads" short for quadrants? And yes, we want to handle the cases mod 4, not just -4 to 4. Thanks for the good review! - Ryan From: owner-vhdl-200x@eda.org [mailto:owner-vhdl-200x@eda.org] On Behalf Of Paul Butler Sent: Monday, June 23, 2014 4:10 PM To: vhdl-200x@eda.org Cc: owner-vhdl-200x@eda.org Subject: Re: [vhdl-200x] Please Review Matrix Math Package The user guide states that the packages are designed for people familiar with matlab. I have never used matlab, so some of this feedback is based on Google searches. 1 - The index types are natural, but positive would be more like matlab. The guide claims this is a concession to C, but it seems illogical to mimic matlab everywhere except in the basic type definitions. On the other hand, it seems possible that synthesizers might implement 0-based index logic more efficiently, but that is not a concession to C. I suggest that the index convention should be "1 to n" to better match matlab. 2 - The guide states that objects declared with a downto range will be "reversed" by the operations. I think the meaning of "reversed" could be misunderstood. A person who thinks of the matrix visually, might expect that the input will be renumbered so that the "upper left" corner will be treated as index (0,0). A person who thinks mostly of the index values may expect the index (0,0) to be treated as the upper-left corner, regardless of the direction. Also, I think that this package does not follow the precedent set by prior VHDL packages regarding the significance of an array's direction vs its index values. For that matter, the LRM specifies that default array operations take inputs left-to-right with relative disregard for the index values. For example: constant uX : unsigned ( 3 downto 0 ) := "0001"; constant uY : unsigned ( 0 to 3 ) := "0001"; constant \uX+uY\ : unsigned := uX + uY; -- = "0010" constant \uX+uX\ : unsigned := uX + uX; -- = "0010" constant mX : real_matrix ( 0 to 1, 0 to 1 ) := (( 1.0, 0.0 ), ( 0.0, 1.0 )); constant mY : real_matrix ( 1 downto 0, 1 downto 0 ) := (( 1.0, 0.0 ), ( 0.0, 1.0 )); constant \mX+mY\ : real_matrix := mX + mY; -- (( 1.0, 1.0 ), -- ( 1.0, 1.0 )) constant \mX+mX\ : real_matrix := mX + mX; -- (( 2.0, 0.0 ), -- ( 0.0, 2.0 )) Because the meaning of "reversed" can be misunderstood, and because this package emphasizes index values over index direction (in contrast to the LRM and other standard packages), I suggest that these operations should throw an error if the inputs fail to comply with the "1 to n" convention. 3 - The submatrix function mimics some matlab indexing options. In matlab, the (i:j) pattern, returns the "slice" starting at index i and ending at index j. The VHDL submatrix function accepts a starting row/column and a number of rows/columns. I suggest mimicking matlab more closely when possible. 4 - In function "*"(matrix, matrix), I think this implementation is equivalent and simpler (a similar transform applies to "*"(vector, matrix)): for i in result'range(1) loop for j in result'range(2) loop result (i, j) := 0.0; --result (i, j) := l(i+l'low(1), l'low(2)) * r(r'low(1), j+r'low(2)); for k in 0 to l'length(2)-1 loop result (i, j) := result (i, j) + (l(i+l'low(1), k+l'low(2)) * r(k+r'low(1), j+r'low(2))); end loop; -- k end loop; -- j end loop; -- i 5 - As it is written, calling "/"(matrix, matrix) with malformed inputs will produce a confusing message about the requirements of matrix multiplication. 6 - "/"(matrix, matrix) is written by calling l * inv(r). I'm surprised that "-"(matrix, matrix) is not written by calling l + ( -r ). 7 - Somebody who knows what every computer scientist should know about floating-point arithmetic should review the implementation for "/"(matrix, real). I'm not sure that ((1.0/R) * L) is always equal to (R/L). 8 - According to the comment in real_matrix_pkg_body.vhd, the purpose of the function "ones" is to return a matrix of zeros -- a copy-paste error, I'm sure. 9 - The recursion for "**"(matrix, integer) will return ones when pow=0. It seems likely that should be "eye". 10 - The recursion for "**" seems too complex. I think the termination for pow=2 could be deleted (assuming the pow=0 should return eye). 11 - The exclude function has a purpose comment for the SubMatrix function. 12 - The SubMatrix function has parameters x, y where the exclude function has parameters row, column serving the same purpose. 13 - The Submatrix has a comment that refers to "position l,r" but the function has no parameters named l and r. 14 - The BuildMatrix has a comment that refers to "position l, r" but has no parameters with those names. 15 - BuildMatrix has commented out the error report statements. 16 - Many functions have some input parameters that are explicitly constant while other parameters are implicitly constant. Why? 17 - Many comments do not correspond to the adjacent code. 18 - rot90(arg, dim) seems to correspond to matlab's rot90(A, k). Your implementation does nothing if dim is outside the range -3 to 3. It seems likely you want to assert that dim is in your supported range or you want to modulo dim into +/- 3. Paul Paul.Butler@ni.com | (512) 683-8743 | National Instruments | Austin, TX From: Jim Lewis <jim@synthworks.com> To: "vhdl-200x@eda.org" <vhdl-200x@eda.org>, Date: 06/22/2014 01:31 PM Subject: [vhdl-200x] Please Review Matrix Math Package Sent by: owner-vhdl-200x@eda.org ________________________________ Hi, Please review the proposed matrix math package. It is proposed to become part of IEEE 1076 and is a necessary part for the 1076.1 VHDL-AMS standard. You can find the user guide here: http://www.vhdl.org/fphdl/real_matrix_ug.pdf You can find the packages here: http://www.vhdl.org/fphdl/real_matrix_pkg.zip Speak now and be heard - otherwise don't complain later. Best Regards, Jim -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Jim Lewis Jim@SynthWorks.com VHDL Training Expert http://www.SynthWorks.com<http://www.synthworks.com/> IEEE VHDL Working Group Chair OSVVM, Chief Architect and Cofounder 1-503-320-0782 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -- This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean. -- This message has been scanned for viruses and dangerous content by MailScanner<http://www.mailscanner.info/>, and is believed to be clean. -- This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean.Received on Wed Jun 25 11:01:51 2014
This archive was generated by hypermail 2.1.8 : Wed Jun 25 2014 - 11:01:51 PDT