Improvements to pl-matrix-output element

[eliving2]

Added Mathematica tab to matrix-output
Added comment capability as a local (optional) parameter named params-comment, works with all three coding languages

[mwest1066]

@eliving2 would you mind adding a new example question that shows off this element? See examplesNumberInput for an example question that demonstrates all the features of a single element. It would be great to have an examplesMatrixOutput that does the same thing, and this would also make easier for me to test and review. Thanks!

[mfsilva22]

It seems that TAM 251 is using this element to display any variables, and not just matrices. To avoid getting other PL users confused, I think we should create an example question that highlights the features of pl-matrix-output mostly displaying matrices, and only one or two parts of the question displaying variables (otherwise I think it will be weird to have an element called "matrix output" and all the examples showing scalar variables).

[mwest1066]

@mfsilva22 any particular reason this PR was closed?

[eliving2]

@mfsilva22 @mwest1066 I noticed a mistake while adding attributes for tab display and finishing the example question (examplesMatrixOutput).

I use pl.string_from_2darray(var_data, language='matlab') for Mathematica because I wasn't thinking about outputting matrices. Apparently, Mathematica syntax is: mat = {{1, 2, 3}, {4, 5, 6}, {7, 8, 9}}...should I edit pl.string_from_2darray as part of this PR?

[mwest1066]

I agree that adding language='mathematica' support to pl.string_from_2darray() would be the right thing to handle this correctly. Including it in this PR would make sense. Thanks!

[mwest1066]

The example question is awesome! It really helped me to review the code better. Thanks!

[mwest1066]

Two other things:

1. As @mfsilva22 points out above, the name for this element is not really appropriate anymore. Should we rename it to pl-variable-output or something like that?

2. Is there any reason we only support scalars and 2D matrices? Any reason we couldn't easily add 1D vector support?

Sorry to expand the scope of your work by asking these questions :-) But I think it would make this a much more powerful element if it could just do a little bit more?

[mfsilva22]

@mwest1066 funny you are bringing up the 1D array support. We should bring @tbretl to this discussion as well.

When I was implementing pl-matrix-component-output, I really wanted to have the 1d array option, because it makes sense for me. But I was asked to remove it from the implementation, to keep consistency with other elements. So now I need to reshape all my 1d arrays as 2d so I can use the element.

Not that I want the extra work, but I do (continue to) agree that only allowing for 2d and scalars is not the best route.

[eliving2]

@mfsilva22 You closed the PR again :(

[eliving2]

@mwest1066 , taking @mfsilva22 's comment into consideration, should I rename the element to pl-variable-output and add 1D vector support?

If so, how would that work? Would this just be a "new" element and pl-matrix-output would be slowly removed just like PLv2 will eventually not be supported?

[mwest1066]

@eliving2 I think doing a rename to pl-variable-output and adding 1D support is the way to go. It would make this element much more useful. The easiest thing to preserve backwards compatability is to leave a copy of pl-matrix-output in place. This can be the old version (before any of your changes). We should remove all references to pl-matrix-output in the docs and example questions, so no one will know about it. At some point in the future we can get everyone using it to switch to pl-variable-ouput and then delete pl-matrix-output (probably over summer).

[mwest1066]

does the comment below need updating now that 1D arrays are supported?

[mwest1066]

actually, it looks like the comments above need updating as well

[mwest1066]

And the function name should not be string_from_2darray() anymore. Maybe string_from_array()?

[mwest1066]

@tbretl could you take a look at the code in this PR that adds 1D array output to prairielearn.py? I suggested up-thread that we rename string_from_2darray(), so any suggestions about what to call it now would also be good.