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

Improve LaTeX builder #42

Closed
veli-m-andirin opened this issue Nov 2, 2021 · 17 comments
Closed

Improve LaTeX builder #42

veli-m-andirin opened this issue Nov 2, 2021 · 17 comments
Assignees

Comments

@veli-m-andirin
Copy link
Collaborator

In this issue we aim to have a working LaTeX builder that can also handle bibliographies and cross-references.

@veli-m-andirin veli-m-andirin self-assigned this Nov 2, 2021
@veli-m-andirin
Copy link
Collaborator Author

@jmshapir, in reply to this comment, here are some experiments using SCons' own PDF builder and our LaTeX builder:

  • SCons PDF builder:
    • Seems to "work" (complete running without error) when the source files are simple enough. It is interesting that I couldn't run the paper using this builder on my local machine and on the VM, but can run slides from minicourse. However, as we could expect, a single run doesn't compile bibliography references correctly.
    • Running it after a CLI run of pdflatex, nothing seems to change, references don't compile correctly.
    • Running it after a CLI run of pdflatex + bibtex, it works correctly.
  • Our LaTeX builder:
    • It doesn't run (running it first, after pdflatex, or after pdflatex + bibtex doesn't change the result), Without the correction mentioned here, the builder crashes immediately, with that correction, it runs but fails saying that it can't find some of the inputs in tex files, which are actually present.

My thought is that since we seem to have found a working sequence of CLI commands which we call from Python, we'd be able to embed that sequence in the builder and have a working version that can deal with bibliographies and cross-references.

@jmshapir
Copy link
Contributor

jmshapir commented Nov 3, 2021

Thanks @veli-m-andirin!

This:

My thought is that since we seem to have found a working sequence of CLI commands which we call from Python, we'd be able to embed that sequence in the builder and have a working version that can deal with bibliographies and cross-references.

sounds right to me.

Tagging @mcaceresb in case he disagrees.

@jmshapir
Copy link
Contributor

jmshapir commented Nov 3, 2021

(And unless we hear otherwise from @mcaceresb, @veli-m-andirin I think you should feel free to try to implement when time permits.)

@mcaceresb
Copy link
Collaborator

@veli-m-andirin Can you point me to the workaround? I've fiddled with this quite a bit in the past for EventStudy so I'd be keen to see what workflow is working here. Thanks!

@veli-m-andirin
Copy link
Collaborator Author

Thanks @jmshapir @mcaceresb!

@mcaceresb,

  • source/paper/SConscript:
Import('env')

build  = env['CONFIG']['global']['build']['paper']
source = env['CONFIG']['global']['source']['paper']
source_lib = env['CONFIG']['global']['source']['lib']

target = ['#%s/EventStudy.pdf' % build,
          '#%s/EventStudyAppendix.pdf' % build]
env.BuildPython(target = target,
           source = ['#%s/compile_paper.py' % source,
                     '#%s/EventStudy.tex' % source,
                     '#%s/EventStudyAppendix.tex' % source,
                     '#%s/compile_latex.py' % source_lib])

env.Install('#release', '#%s/' % build)
  • source/paper/compile_paper.py:
import os
import glob
import shutil
import sys

sys.path.append('source\lib')
from compile_latex import compile_file

current_dir = os.getcwd()
source_dir  = "source/paper"

os.chdir(source_dir)

compile_file("EventStudy", True)
compile_file("EventStudyAppendix", True)

source_files = "*.pdf"

file_list = glob.glob(source_files)

for output_file in file_list:
    shutil.move(output_file, os.path.join("../../build/paper", output_file))
    
os.chdir(current_dir)
  • source/lib/compile_latex.py:
import os

def compile_file(file_name, has_bib):
    os.system("pdflatex %s" % file_name)
    if has_bib == True:
        os.system("bibtex %s" % file_name)
        os.system("pdflatex %s" % file_name)
    os.system("pdflatex %s" % file_name)

@jmshapir
Copy link
Contributor

jmshapir commented Nov 4, 2021

Thanks @veli-m-andirin!

(And @mcaceresb if you prefer to get a direct look at EventStudy again just let me know and I'm happy to grant access.)

@mcaceresb
Copy link
Collaborator

@veli-m-andirin

  • Without a .bib file, two passes often work, but I think that depending on the nature of the references 3 is the correct number of passes: The first pass does not insert any references; the second pass inserts the references but in doing do might change the document and thus the references might need to be updated; the third pass is the one that should generally be correct. I'd suggest 3 passes if the additional compile time isn't an issue.
  • Even with the above, I believe modern latex compilers like latxmk use an iterative process to figure out the references, (so if only one is needed only one runs, or two, or three, or more). This matters because if it can't figure out the references, then it should give an error saying so. For instance, my recollection is that the scons compiler exits after 4 passes if there are still references to sort out and it tells you that it couldn't get the references right. This can be a useful error to have in the log. The current setup assumes the references are correct or that you will be able to tell if there are errors with the references (this can be annoying with lengthy documents).
  • I would also double-check that the cross-references between EventStuty and EventStudyAppendix are correct. I believe the former references the latter, right? If so, then the updated .aux file needs to be available when EventStudy compiles, so EventStudyAppendix should be compiled first in this case. However, this doesn't deal with the general use case where there are cross-references across documents, in which case the .aux file from one needs to be a dependency for the other so that they compile in the right order.
  • Last, if memory serves there was an issue in this particular repo across operating systems having to do with converting eps figures into a format LaTeX could read. I think the issue was that on my OS the -shell-escape flag was needed but in other OSs it wasn't... In any case, I would double-check the figures are being correctly updated as well.

@rcalvo12
Copy link
Collaborator

@jmshapir, I just wrapped up my first pass at the improved latex builder (!!!!!!!!) but need write access to share my results.

@jmshapir
Copy link
Contributor

@jmshapir, I just wrapped up my first pass at the improved latex builder (!!!!!!!!) but need write access to share my results.

@rcalvo12 done, thanks!

@rcalvo12
Copy link
Collaborator

In 402c380 I produced my first pass at the improved latex builder. I appreciate everyone's patience on this. Changes were made in branch issue42_latex_builder.

The previous latex builder actually wasn't too far off, and after adjusting some fields in the latex file I could produce a pdf from a latex file that included external tables and graphs, footnotes, and a handwritten reference section. The main changes here were to make sure that the files referenced within the latex file were referenced from root of the directory. For example, instead of calling a table with \input{../output/tables/top_gdp} you call it with \input{output/tables/top_gdp}. You can see examples in the TemplateTest.tex file.

However, with the previous version of the builder, we could not use a .bib file to produce a bibliography.

The updated builder uses the following sequence to compile the latex files:

pdflatex -> bibtex -> pdflatex -> pdflatex

If you go to the /issue42/ directory, delete everything except References.bib, TemplateTest.tex, and Sconscript, and then run scons from the root of the repository you can see that it produces the sample pdf with the proper references.

Looking forward to iterating on next versions.

@jmshapir
Copy link
Contributor

Thanks @rcalvo12 and nice work! I was able to compile the issue folder successfully on my first try. :-)

A few things:

  • I think we should add Template.tex to /souce/paper. (That way users of JMSLab/Template have an easy choice between TeX and LyX.)
  • I think we should add References.bib to /source/paper, and we should revise Template.lyx to use BibTeX for references. (I'm inclined to move the lab towards using BibTeX more universally.)
  • I think we should update test_build_latex.py to test the current instantiation of the builder.

From my standpoint, once we've done those things, you can open a pull request with @veli-m-andirin @jmshapir as reviewers. (At that point we can also see if @mcaceresb wants to review.)

@rcalvo12
Copy link
Collaborator

  • I think we should add Template.tex to /souce/paper. (That way users of JMSLab/Template have an easy choice between TeX and LyX.)
  • I think we should add References.bib to /source/paper, and we should revise Template.lyx to use BibTeX for references. (I'm inclined to move the lab towards using BibTeX more universally.)
  • I think we should update test_build_latex.py to test the current instantiation of the builder.

@jmshapir I moved Template.tex and References.bib to /source/paper and I have revised Template.lyx to use BibTeX for references. I also modified sconscript files so that outputs for both Template.tex and Template.lyx are sent to /output/paper when scons is run at the root of the directory.

As far as updating test_build_latex.py, I'm not sure what extra tests we would need here or which tests would need to be modified. It seems to me that the way test_build_latex.py is constructed would still work, though I may be wrong about that.

@jmshapir
Copy link
Contributor

As far as updating test_build_latex.py, I'm not sure what extra tests we would need here or which tests would need to be modified. It seems to me that the way test_build_latex.py is constructed would still work, though I may be wrong about that.

@rcalvo12 thanks! If the existing unit tests for build_latex still work, and cover the main elements we're testing for in the unit tests for our other builders, then we should be all set here. Just wanted to make sure unit tests are functioning and up to date for this revised builder!

@rcalvo12
Copy link
Collaborator

rcalvo12 commented Apr 1, 2022

@jmshapir Checking it against some of the other builders, and keeping in mind the changes that were made, I think we should probably be fine with the current test. I can flag this as a point for review in the PR though since, again I might be missing something. With that in mind, do you think we're ready for a PR on this?

@jmshapir
Copy link
Contributor

jmshapir commented Apr 1, 2022

@rcalvo12 sounds good and yes!

@rcalvo12
Copy link
Collaborator

rcalvo12 commented Apr 1, 2022

Thread continues in PR #56.

rcalvo12 added a commit that referenced this issue Apr 4, 2022
mcaceresb added a commit that referenced this issue Apr 4, 2022
rcalvo12 added a commit that referenced this issue Apr 15, 2022
rcalvo12 added a commit that referenced this issue Apr 22, 2022
rcalvo12 added a commit that referenced this issue Jul 3, 2022
rcalvo12 added a commit that referenced this issue Aug 16, 2022
rcalvo12 added a commit that referenced this issue Aug 24, 2022
rcalvo12 added a commit that referenced this issue Aug 24, 2022
rcalvo12 added a commit that referenced this issue Sep 22, 2022
rcalvo12 added a commit that referenced this issue Sep 23, 2022
* #42 Full run of repo with improved latex builder

* #42 Tweak to builder to allow for different target and source name

* #42 Moves Template and References to source/paper/

* #42 Revise Template.lyx to use bib references

* #56 #42 Drop issue folder from Scons

* #56 #42 Add bib log files to gitignore

* #56 #42 Rename plot.m to makeplot.m to avoid error

* #56 #42 Ignoring all tex log but not sconscript.log

* #56 #42 Adding .bib file to scons

* #56 #42 Removing old sconscript file

* #56 #42 Cleaning add_bib_name()

* #56 #42 Only calls bibtex when bibliography is used

* #42 Added example where 2 runs fail

* #56 #42 Cleaning check_bib() moving checks to do_call()

* #56 #42 Reversing (target, source) to match

* #56 #42 Adding in cleanup function for .aux .bbl

* #56 #42 Cleaner version of cleanup function

* #56 #42 Adding other file types to cleanup

* #56 #42 Adding ability to change bibtex executable

* #56  #42 Cleaning up before compiling

* #56 #42 Update tests for multiple calls

* #42 Added out name before trying to execute call

* #56 #42 Empty bib side effect and small edit

* #56 #42 Adding basic regex and connect to side effect

* #56 #42 Correcting regex

* #56 #42 Fixing how we switch to bib side_effect

* #56 #42 Adding test tex files

* #42 Added mock bibtex test; modified standard test to take nsyscalls

* #42 Adding Latex to SConstruct, test running

* #42 Removing issue sub

* #42 Removing files we don't want to track

* #42 Gitignoring pdfs in output/paper

* #42 Removing outputs and rerunning, removing pdfs, changing where pdfs ignored

* #42 Removing extra file

Co-authored-by: Mauricio Caceres <mauricio.caceres.bravo@gmail.com>
Co-authored-by: jmshapir <jesse.m.shapiro@gmail.com>
@rcalvo12
Copy link
Collaborator

Summary:

In this issue we fixed our LaTeX builder so that we can now use scons to convert LaTeX files to PDF, including LaTeX files with corresponding bibtex files for references. Paper's are compiled in the following way under the surface:

pdflatex -> pdflatex -> pdflatex #No Bibtex
pdflatex -> bibtex -> pdflatex -> pdflatex #With Bibtex

We also added testing for the LaTeX builder and the bibtex functionality.

Final state of issue branch here.

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

No branches or pull requests

4 participants