Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make default matrix type a global option? #62

Open
friendly opened this issue Sep 3, 2024 · 16 comments
Open

Make default matrix type a global option? #62

friendly opened this issue Sep 3, 2024 · 16 comments

Comments

@friendly
Copy link
Owner

friendly commented Sep 3, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

friendly commented Sep 3, 2024

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

@john-d-fox
Copy link
Collaborator

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

@john-d-fox
Copy link
Collaborator

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 john-d-fox reopened this Sep 4, 2024
@john-d-fox
Copy link
Collaborator

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

@friendly
Copy link
Owner Author

friendly commented Sep 4, 2024

@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
Copy link
Owner Author

friendly commented Sep 4, 2024

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
Copy link
Collaborator

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
Copy link
Owner Author

friendly commented Sep 4, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants