friendly / matlib

Matrix Functions for Teaching and Learning Linear Algebra and Multivariate Statistics
http://friendly.github.io/matlib/
65 stars 16 forks source link

Make default `matrix` type a global option? #62

Open friendly opened 2 months ago

friendly commented 2 months ago

Just was we did with the default latexMultSymbol, it would be useful to allow an option() to change the default matrix type (pmatrix, bmatrix, Bmatrix, ...)

This is straightforward if we allow anything to be assigned and used in latexMatrix() and friends. But would this need to be checked in functions against any list of possibilities?

john-d-fox commented 2 months ago

This seems a reasonable thing to do. I'm working on something else now so can't get to it right away.

I think that the only thing that would have to be changed is the default value of the matrix argument to latexMatrix(), from matrix = "pmatrix" to something like matrix = getOption("matrix"). If the latter returnsNULL, then matrix should be set to "pmatrix" inside latexMatrix(), i.e., if (is.null(matrix)) matrix <- "pmatrix".

Everything else should just work. Of course, this has to be checked carefully, which is what I can't do now.

john-d-fox commented 2 months ago

Oh. About checking matrix against a list of possibilities: That's a good idea, but it isn't done currently. That is, the matrix arg could at present be set to nonsense without being flagged.

friendly commented 2 months ago

On the other hand, one could define a \newenvironment{foomatrix} and then want to use that

john-d-fox commented 2 months ago

Please feel free to implement it either way -- I doubt whether it would make much difference, and both appear simple,

john-d-fox commented 2 months ago

I had some time this morning, so I introduced a "latexMatrixEnv" option that defaults to "pmatrix" if the option isn't set. This required only small changes to latexMatrix() and updateWrapper(). The value of the option isn't checked against a list of allowed values.

I see that R CMD check fails with

> # with Quarto
  > Eqn('e = mc^2', label='eq-einstein', quarto=TRUE)

  $$
  e = mc^2
  $$ {#eq-einstein}
  > ref('eq-einstein', quarto=TRUE)
  Error in ref("eq-einstein", quarto = TRUE) : 
    unused argument (quarto = TRUE)
  Execution halted

I assume that this relates to the issue Eqn parser #59 and has nothing to do with the introduction of the "latexMatrixEnv" option.

john-d-fox commented 2 months ago

Sorry -- I accidentally closed this issue and so reopened it.

friendly commented 2 months ago

@john-d-fox I was just now working on the same global matrix option, but haven't committed, so I'll just git stash that.

I also ran into the same error with unused argument (quarto = TRUE)

friendly commented 2 months ago

Aside: an elegant way to handle an option or value that may be null, using a default value is

matrix <- getOption("latexMatrixEnv") %||% "pmatrix"

but I think %||% was introduced in R 4.4

john-d-fox commented 2 months ago

Apologies for preempting your work on the "latexMatrixEnv" option. I didn't realize that you were working on it, but of course I suggested that yesterday. Please feel free to substitute your work for mine -- but be careful to modify updateWrapper() as well as latexMatrix(); a search in R/ suggests that these are the only two places where pmatrix is hard-coded.

I was unaware of %||%; thanks for drawing my attention to it. In any even, I think that matrix = getOption("latexMatrixEnv") should be in the arguments of latexMatrix(), so that the user sees where the default comes from, and the check for NULL should be in the body of the function.

With respect to R CMD check failing, I don't understand the problem because Eqn() does have a quarto argument, but I guess I'll let you and Phil sort that out because I haven't really paid attention to the development of Eqn()

friendly commented 2 months ago

No problem with using your version; I had just started on this when I saw your note here. Posting what we do is a good way to avoid conflicts.

john-d-fox commented 2 months ago

Looking again at the R CMD check error, I see that it's ref(), not quarto() that's being called with the quarto argument at the point of the error (ref('eq-einstein', quarto=TRUE)). Maybe that hint will help.

My general practice is not to commit changes until R CMD check --as-cran --run-donttest is clean. I proceeded in this case because I was pretty sure that the error wasn't in my code.

philchalmers commented 2 months ago

Just catching up on this thread. Coincidentally I fixed this earlier as the quarto logical in ref() is not useful anymore, unless it's possible to make inline markdown expressions report 'asis' results, though I inadvertently left some detritus in an roxygen example. Should be patched now.

philchalmers commented 2 months ago

validateMatrix() internal function added to check for non-sensical matrix inputs, including the in-line possibilities if mathtools is loaded (https://tex.stackexchange.com/questions/345882/how-do-i-typeset-a-2x2-matrix-in-an-inline-equation). I'm hesitant about allowing smallmatrix through as IMO it makes less sense in this context (would require users to wrap the objects with additional calls like cat('\\bigl') and such), but it's allowed for now in case a borderless matrix is really the desired output.

friendly commented 2 months ago

It is certainly worth checking matrix types, but should something outside the list be a warning or error? There are quite a few other matrix-like environments in other packages, but I don't see a real need to cater to them now

philchalmers commented 2 months ago

Would you prefer something like latexMatrix(..., validate = FALSE) to disable checks such as this? The current validate approach covers 99%+ of the common cases, so does more good then harm, but I agree turning checks off would be desirable and reverts to the original behavior.

friendly commented 2 months ago

I'm OK with leaving it as is, ie always validate; if some use case arises to do otherwise, can always revisit this