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

Insufficient libraries get loaded in some of the code examples in the pgfmanual v3.1.8b #971

Closed
marmotghost opened this issue Jan 1, 2021 · 18 comments

Comments

@marmotghost
Copy link

Concerns manual v3.1.8b. Several code examples load insufficient libraries. (No, I do not have a complete list. Creating a complete list would require to look at the code examples one by one and see if the output matches the results in the pgfmanual.)

Example from p 269.

\documentclass[tikz,border=3mm]{standalone}
\usetikzlibrary{graphs}
\begin{document}
\tikz
\graph [nodes={draw, circle}, clockwise, radius=.5cm, empty nodes, n=5] {
subgraph I_n [name=inner] --[complete bipartite]
subgraph I_n [name=outer] };
\end{document}

yields
Screen Shot 2020-12-31 at 1 36 54 PM

Replace \usetikzlibrary{graphs} by \usetikzlibrary{graphs.standard} and get the result depicted in the pgfmanual.
Screen Shot 2020-12-31 at 1 37 16 PM
The next code example has the same problem, and probably many more.

@marmotghost marmotghost changed the title Insufficient libraries get loaded in the code examples in the pgfmanual v3.1.8b Insufficient libraries get loaded in some of the code examples in the pgfmanual v3.1.8b Jan 1, 2021
@muzimuzhi
Copy link
Member

muzimuzhi commented Jan 1, 2021

Part of the task of #640. That is, extend docexamples codeexamples to MWE and assert the added dependencies are both sufficient and necessary.

@marmotghost
Copy link
Author

marmotghost commented Jan 1, 2021

@muzimuzhi Yes, but please note that in the case at hand one cannot rely on getting an error when one does not include the relevant libraries, one would have to visually compare the output of the code example with the result shown in the pgfmanual. Most of the examples in the pgfmanual do specify the relevant libraries, but these here do not.

@Mo-Gul
Copy link
Contributor

Mo-Gul commented Jan 1, 2021

From my mind and just for the record:
I don't know the full story any more, but the idea was to build a(n automated) test suite (see e.g. issue #666) similar to the one that is used for PGFPlots. That is, build reference images of each codeexample and do "pixel compares" with the ones created from issue #640.

Christian Feuersänger offered his help for implementing this also for pgf and there was also already was a Teams meeting in June 2020 with Henri, Christian and me about this. The consensus was that this will be postponed until Henri has finished his ... (not sure if I am allowed to tell this).

This should be the case now, but maybe Henri forgot about that.?

@marmotghost
Copy link
Author

@Mo-Gul Yes. The problem with the above ones is that they can easily fall through the cracks because you do get some result if you do not load all relevant libraries. That is, you really have to know what the "correct" result is beforehand to be able to judge that the code example is working.
Please note that this case is also a bit subtle in that some of the graphs, but not these, require lualatex, so the first reaction of a user may be to try out lualatex and then falsely suspect that the LaTeX kernel is to blame because this is what happened in the tikz-feynman saga. And these particular examples are the first occurrences of C_n and I_n in the whole manual, and if already those "do not work" then this might deter users from considering to make use of the graphs libraries with all their sub libraries.

Happy New Year!

@hmenke
Copy link
Member

hmenke commented Jan 1, 2021

Duplicate of #640.

@hmenke
Copy link
Member

hmenke commented Jan 1, 2021

As you have already noted, addressing this consistently for all examples in the manual is a gargantuan task. At the same time there is no way to assert this in the CI because as you pointed out the compilation is successful but the output is wrong and more importantly because compiling each code example from the manual individually takes several hours.

@marmotghost
Copy link
Author

Of course I disagree with the assertion that this is a duplicate. Yet, leaving this aside, there is a real issue here: the users get rubbish if they copy a snippet from the manual and try to get the results with the libraries indicated. At the very least one could fix the examples which have already been pointed out. Otherwise others who stumble over the same problem may find this thread and see what happened to it. So from my perspective fixing a subset of the examples is still better than making no changes for a while. (In this case it is very simple: look for C_n, I_n I_nm in the pgfmanual and check if the graphs.standard library gets loaded. There are several hits.)

@hmenke
Copy link
Member

hmenke commented Jan 1, 2021

Have you looked at #883? No, of course you haven't.

@marmotghost
Copy link
Author

All I am saying is that there is a problem with some code examples. You can of course do whatever you like with this information, yet it would be great if you could just refrain from making unsubstantiated statements like "No, of course you haven't.". (Just for the records, I did have a look at this but it does not solve the above-mentioned problem.)

@hmenke
Copy link
Member

hmenke commented Jan 1, 2021

See you next week.

@pgf-tikz pgf-tikz temporarily blocked marmotghost Jan 1, 2021
@Mo-Gul
Copy link
Contributor

Mo-Gul commented Jan 1, 2021

As you have already noted, addressing this consistently for all examples in the manual is a gargantuan task. At the same time there is no way to assert this in the CI because as you pointed out the compilation is successful but the output is wrong and more importantly because compiling each code example from the manual individually takes several hours.

@hmenke, just to be sure I got it right:
When there would be "Christian's pixel comparison" available as GitHub Action I volunteer to check each codeexample result so we have a proper reference for later automated comparisons. Don't you think any more that this would work or do you have (in the meantime) a better idea?

I agree to marmot that the already known wrong examples should be fixed. Shall I propose a pull request?

@hmenke
Copy link
Member

hmenke commented Jan 1, 2021

First I have to get around to extracting the examples from the Lua documentation. Then we can check and merge #883 which already fixes the present issue (as far as I can see).

Instead of pixel comparison I would go for l3build and compare sanitized PDFs which is faster and directly points to the error. That should be fairly simple to set up, I just haven't done that yet. But even then I would rather go for curating examples that test certain behavior instead of just brute-forcing all code examples from the manual which will take hours to complete.

@muzimuzhi
Copy link
Member

Yet, leaving this aside, there is a real issue here: the users get rubbish if they copy a snippet from the manual and try to get the results with the libraries indicated. (#971 (comment))

An other idea is to add a safe guard for lib graphs.standard in graphs, so users will get helpful error messages instead of degenerated graphs.

diff --git a/tex/generic/pgf/frontendlayer/tikz/libraries/graphs/tikzlibrarygraphs.code.tex b/tex/generic/pgf/frontendlayer/tikz/libraries/graphs/tikzlibrarygraphs.code.tex
index ce7db967..264e818e 100644
--- a/tex/generic/pgf/frontendlayer/tikz/libraries/graphs/tikzlibrarygraphs.code.tex
+++ b/tex/generic/pgf/frontendlayer/tikz/libraries/graphs/tikzlibrarygraphs.code.tex
@@ -1365,6 +1365,14 @@
   declare/.code 2 args={\expandafter\def\csname tikz@lib@graph@def@#1\endcsname{\tikz@lib@graph@do@graph{#2}}}%
 }%
 
+% These macros will be overwritten in lib "graphs.standard"
+\pgfutil@for\pgf@temp:=I_n,I_nm,K_n,K_nm,P_n,C_n,Grid_n,G_np\do{
+  \expandafter\edef\csname tikz@lib@graph@def@subgraph \pgf@temp\endcsname{%
+    \noexpand\tikzerror{You need to say \string\usetikzlibrary{graphs.standard}
+      to use graph macro ``subgraph \detokenize\expandafter{\pgf@temp}''}%
+  }
+}
+
 \def\tikz@lib@graph@handle@graph#1{%
   \begingroup%
     \let\tikz@lib@graph@node@list\pgfutil@empty%

At the same time there is no way to assert this in the CI [...] and more importantly because compiling each code example from the manual individually takes several hours. (#971 (comment))

@hmenke Compiling the code examples individually could be triggered on a less frequent basis. So finally and hopefully pgf will have a comprehensive test suite triggered per-commit/push and another test suit triggered by, for example pre-releases.

@Mo-Gul
Copy link
Contributor

Mo-Gul commented Jan 2, 2021

You are right, at least some examples will be fixed by #883. (I didn't check if there could be more.) When you don't see a progress for remaining/improved extraction of the Lua examples, maybe the PR could already be merged and maybe another issue for the remaining stuff could be opened (it not #640 is enough as remainder?

I agree that the pixel comparison and a "brute-force" check of all code examples are suboptimal, but until we have something else it would be better than nothing at all. I personally won't mind if it takes hours, because it is unlikely to take more than one night. It could also be made as GitHub Action that is not triggered automatically or only for releases (if possible).

Of course -- as always -- you are the pro and would be the person in charge to program that, so it is your decision.

@hmenke
Copy link
Member

hmenke commented Jan 2, 2021

I've already merged #883 in parts, but I didn't want to merge the Lua part before it can be tested.

I'm strongly against a test suite that runs several hours. It has to be possible to quickly run the tests locally. Ideally this would even be restricted to only the subsystems that changed. Also note that GitHub charges money if you use more than 2000 build minutes per month (same holds for all other CI providers).

@ilayn
Copy link
Member

ilayn commented Jan 2, 2021

without pickling the preamble I think this type of testing won't fly anyways. If tests finish under an hour things are pretty safe which implies division of tests into parts (which we did for ARM architecture since it is too slow https://github.com/scipy/scipy/pull/13328/checks?check_run_id=1637581154) and some parallelization magic. You can mark certain parts as slow anyways.

But the testing orchestration file open closes etc cannot be done in TeX that's just shooting yourself in the foot with your foot. But I think you didn't want that either so Lua might be fine Python also fine, if you are going to string compare some PDFs (for which I have some reservations. For some reason system level output comparison seems better to me but anyways no strong opinion)

@hmenke
Copy link
Member

hmenke commented Jan 3, 2021

You can't dump Lua code into a TeX format, so pickling the preamble will not be possible for LuaTeX, but due to graph drawing this is the engine with biggest exposure.

Orchestration will ideally be done by l3build which is written and configured in Lua. String comparison of PDFs is necessary for anything that uses PDF objects, e.g. patterns, fadings.

@muzimuzhi
Copy link
Member

Also note that GitHub charges money if you use more than 2000 build minutes per month (same holds for all other CI providers).
#971 (comment)

While learning GitHub Actions, I find the doc says GitHub Actions is free for public repositories.

GitHub Actions usage is free for public repositories and self-hosted runners. For private repositories, each GitHub account receives a certain amount of free minutes and storage, depending on the product used with the account. Any usage beyond the included amounts is controlled by spending limits.
About billing for GitHub Actions - GitHub Docs

And that seems to be true since Aug 2019.

..., so Actions is free for the 40 million developers on GitHub to use with public repositories.
GitHub Actions now supports CI/CD, free for public repositories - GitHub Blog

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

No branches or pull requests

5 participants