Skip to content

first set of pre-commit hooks #57

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tschm
Copy link
Contributor

@tschm tschm commented Apr 11, 2025

No description provided.

@tschm
Copy link
Contributor Author

tschm commented Apr 11, 2025

@jonathan-taylor pre-commit hooks are a powerful tool. Try to apply them with “make fmt”. I have commented hooks that would have at this stage a lot of differences. It’s very common to take notebooks under version control but strip all there output before. This could be achieved using a pre-commit hook, e.g. a little helper that runs before committing into the repo. Usually used for linting, etc.

@jonathan-taylor
Copy link
Collaborator

Agree this is a good idea. One thing we should decide is nbstripout. I think there's some value in having currently rendered ipynb files so students can go to github and see them close to the format they'd see in jupyter lab.

@jonathan-taylor
Copy link
Collaborator

jonathan-taylor commented Apr 11, 2025

I also just went through and changed the ipynb / Rmd files to get rid of LaTeX references. I wonder if these should perhaps remain in the source (can be reverted easily at this point) and we just publish branches of rendered ipynb (with references fixed) as we may do with the Rmd files.

This is part of the transition from having the LaTeX as source and moving to .ipynb as source.

@tschm
Copy link
Contributor Author

tschm commented Apr 12, 2025

Yes, if it can be automated it should be done. The rmd job is an example and variations are possible

@tschm
Copy link
Contributor Author

tschm commented Apr 12, 2025

We could also compile LaTeX documents during ci/cd. Let me find an example

@tschm
Copy link
Contributor Author

tschm commented Apr 12, 2025

'''

me: LATEX

on:
push

write to the draft branch

permissions:
contents: write

jobs:
latex:
runs-on: ubuntu-latest
steps:
- name: "Build and publish the LaTeX document"
uses: tschm/cradle/actions/[email protected]
with:
paper: paper/document.tex
'''

@trevorhastie
Copy link
Collaborator

I am getting a little lost here. The latex source for the labs generated the .Rmd files and hence the . ipynb files via jupytext.
This was done using a detailed R script that I wrote.
Jonathan and I agreed that going forward he would edit the .Rmd files and, and at some point I would reconcile the latex files so that my script would reproduce the current .Rmd. This would most likely be at a point went we wanted to produce a new pdf of the book.
In light of this, I am trying to understand the discussion here about latex

@tschm
Copy link
Contributor Author

tschm commented Apr 13, 2025

I don't know the full context. I thought we accept the ipynb files as source as we can test them via workflows. From the ipynb files we can create the rmd files automatically via a GitHub workflow. This avoids that we need to go through any manual syncing.

We can also create LaTeX files via nbconvert. Details are given here: https://saturncloud.io/blog/how-to-use-latex-in-jupyter-notebook/ Again, this could be done in a workflow.

So the problem are the links to sections in the big global LateX document?

@tschm tschm marked this pull request as draft April 13, 2025 19:03
@tschm tschm marked this pull request as ready for review April 14, 2025 02:52
@jonathan-taylor
Copy link
Collaborator

jonathan-taylor commented Apr 14, 2025

It's just that the page here: github.com/intro-stat-learning/ISLP_labs probably shouldn't have LateX references like \ref, \eqref, \pageref etc. While the true LaTeX does. Do we want to keep those refs in the .ipynb source (and convert them at build) or just strip them from the source.

There are a few \pageref calls in the labs .ipnb files that have been hard-coded in our current source. These pages would probably change if we ever update the PDF. We'd then have to go back and find where they should be in the ipynb files.

So the issue is not LaTeX files but LaTeX refs in .ipynb files

@tschm
Copy link
Contributor Author

tschm commented Apr 14, 2025

So the LaTeX ref in an ipynb file is somewhat hard to deal with. I would only include generic {{ }} references and replace them using jinja.

So in a ipynb file in a notebook I would have

Hello {{ name }}, your fav. equation is {{ equation }}

I think LaTeX should be able to compile the notebook still with those {{ }} links. You can then load the tex file as a jinja template and replace them all (or you do that before you compile).

@jonathan-taylor
Copy link
Collaborator

We can do the replace pretty easily, but yes jinja might be easier. We'd just have to keep a copy of the current .aux files around to do fill in the template (or some namespace with the values for the template). This way at least we could name the items in the jinja template the same names as the LaTeX.

There is still the issue of the source .ipynb containing refs, so it does maybe make sense to simply have a "published" branch of .ipynb files with the values filled in, similar to what @tschm is proposing for Rmd.

@tschm
Copy link
Contributor Author

tschm commented Apr 14, 2025

Yes, I would opt for the master notebook with {{ }} suitable for jinja.

And then 3 branches for ipynb, rmd, and Tex files where we fill the template with suitable values

Should be easy and no aux files needed

@tschm
Copy link
Contributor Author

tschm commented Apr 16, 2025

@jonathan-taylor Could we have a brief call? Zoom? Google Meet?

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

Successfully merging this pull request may close these issues.

3 participants