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

Delay LaTeX feature tests as long as possible #39430

Merged
merged 8 commits into from
Feb 10, 2025

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Feb 2, 2025

We shouldn't need a functional LaTeX engine to turn the matrix [1] into \left(\begin{array}{r}1\end{array}\right). We used to try; now we don't. (This avoids compiling a test document in a temporary directory.)

There's some other cleanup at the same time, but the basic idea is to avoid putting the result default_engine() in the preferences dict when the preferences dict is constructed. Instead, we now leave the "engine" option set to None by default. Inside the functions where latex is actually used, we check...

  1. Did the user pass engine=whatever to this function?
  2. If not, is the "engine" option set in the preferences dict?
  3. If not, use the default engine.

So foo(engine=bar) overrides the preferences dict, and the preferences dict overrides default_engine().

Closes:

This does nothing, except waste your time trying to ensure that it is
consistent with the "engine" property and that It iS PrOpErLy
CapItALizEd.
This delays probing the default engine (which tries to compile some
LaTeX code in a temporary directory) when you merely wish to query
e.g. the matrix delimiters. A later commit will re-insert the probe
at a later point.

The doctest for the options dict was updated to print a sorted list of
items, so now its output is no longer random.
When we try to find the current LaTeX engine, the preferences dict
will now have __options["engine"] == None to indicate that we should
fall back to the default engine. In each case we check for None, and
invoke default_engine() only if necessary.

This allows us to query the options dict without triggering the
expensive is_functional() method for the LaTeX features.
We should not need to test the functionality of various engines before
we can e.g. obtain the latex representation of a matrix. We add a
doctest to ensure that this is the case.

Closes sagemathGH-39351
This function is relatively new and is only used in sage.misc.latex.
I don't think there is a compelling need to have it as part of the public
API, so let's give it an underscore.
@orlitzky orlitzky force-pushed the lazier-latex-feature-tests-pr branch from 8dcf996 to 8ed829c Compare February 2, 2025 03:44
@orlitzky
Copy link
Contributor Author

orlitzky commented Feb 2, 2025

Force push was to change TESTS:: to TESTS: and hopefully fix the CI

Copy link

github-actions bot commented Feb 2, 2025

Documentation preview for this PR (built with commit 6eed1bc; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky
Copy link
Contributor Author

orlitzky commented Feb 2, 2025

The conda CI failure is unrelated

In case anyone is using this, we avoid removing it (or changing its
return type) by surprise, and instead deprecate it.

changes. Lines starting # with '#' will be ignored, and an empty
message aborts the commit.  # # On branch lazier-latex-feature-tests #
Changes to be committed: # modified: src/sage/misc/latex.py #
This function no longer returns the stylized engine name, so its
docstring needs an update to reflect that.
(This appeases pycodestyle, and makes the docstring endings more
consistent.)
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 3, 2025

I suggest posting the semantic change on the latex preferences in https://github.com/sagemath/sage/wiki/Sage-10.6-Release-Tour

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 4, 2025
sagemathgh-39430: Delay LaTeX feature tests as long as possible
    
We shouldn't need a functional LaTeX engine to turn the matrix `[1]`
into `\left(\begin{array}{r}1\end{array}\right)`. We used to try; now we
don't. (This avoids compiling a test document in a temporary directory.)

There's some other cleanup at the same time, but the basic idea is to
avoid putting the result `default_engine()` in the preferences dict when
the preferences dict is constructed. Instead, we now leave the "engine"
option set to `None` by default. Inside the functions where latex is
actually used, we check...

1. Did the user pass `engine=whatever` to this function?
2. If not, is the "engine" option set in the preferences dict?
3. If not, use the default engine.

So `foo(engine=bar)` overrides the preferences dict, and the preferences
dict overrides `default_engine()`.

Closes:

* sagemath#39351
    
URL: sagemath#39430
Reported by: Michael Orlitzky
Reviewer(s): Kwankyu Lee, Michael Orlitzky, user202729
@orlitzky
Copy link
Contributor Author

orlitzky commented Feb 4, 2025

Thank you both.

I suggest posting the semantic change on the latex preferences in https://github.com/sagemath/sage/wiki/Sage-10.6-Release-Tour

I don't think there is a semantic change? You can still set engine_name in the preferences dict, and as before, it will have no effect on anything. The default engine itself hasn't changed, nor has the way you override it. The only difference is that before, the default was stored in the preferences dict upon access, and now it isn't (we leave it None). You still set your preferred engine in exactly the same way however.

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 4, 2025

... The only difference is that before, the default was stored in the preferences dict upon access, and now it isn't (we leave it None).

I meant this. Some may be confused with "None", which they didn't see before.

Anyway, that is a mild suggestion.

@orlitzky
Copy link
Contributor Author

orlitzky commented Feb 9, 2025

I suggest posting the semantic change on the latex preferences in https://github.com/sagemath/sage/wiki/Sage-10.6-Release-Tour

Done!

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39430: Delay LaTeX feature tests as long as possible
    
We shouldn't need a functional LaTeX engine to turn the matrix `[1]`
into `\left(\begin{array}{r}1\end{array}\right)`. We used to try; now we
don't. (This avoids compiling a test document in a temporary directory.)

There's some other cleanup at the same time, but the basic idea is to
avoid putting the result `default_engine()` in the preferences dict when
the preferences dict is constructed. Instead, we now leave the "engine"
option set to `None` by default. Inside the functions where latex is
actually used, we check...

1. Did the user pass `engine=whatever` to this function?
2. If not, is the "engine" option set in the preferences dict?
3. If not, use the default engine.

So `foo(engine=bar)` overrides the preferences dict, and the preferences
dict overrides `default_engine()`.

Closes:

* sagemath#39351
    
URL: sagemath#39430
Reported by: Michael Orlitzky
Reviewer(s): Kwankyu Lee, Michael Orlitzky, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39430: Delay LaTeX feature tests as long as possible
    
We shouldn't need a functional LaTeX engine to turn the matrix `[1]`
into `\left(\begin{array}{r}1\end{array}\right)`. We used to try; now we
don't. (This avoids compiling a test document in a temporary directory.)

There's some other cleanup at the same time, but the basic idea is to
avoid putting the result `default_engine()` in the preferences dict when
the preferences dict is constructed. Instead, we now leave the "engine"
option set to `None` by default. Inside the functions where latex is
actually used, we check...

1. Did the user pass `engine=whatever` to this function?
2. If not, is the "engine" option set in the preferences dict?
3. If not, use the default engine.

So `foo(engine=bar)` overrides the preferences dict, and the preferences
dict overrides `default_engine()`.

Closes:

* sagemath#39351
    
URL: sagemath#39430
Reported by: Michael Orlitzky
Reviewer(s): Kwankyu Lee, Michael Orlitzky, user202729
@vbraun vbraun merged commit 462b69a into sagemath:develop Feb 10, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants