Geometric series lecture (WIP)

[DawievLill]

I have started with the geometric series lecture and wanted to ask a few questions, based on this example, before I complete the entire thing.

1. Have I created the branch correctly to be merged with the main repository? Or have I made some mistake?
2. Do I have to execute the Jupyter Book locally before I submit a PR, or can I only make changes to the markdown file and submit.
3. What happens if I want to introduce a package that is not currently listed in the project .toml? For example, for this lecture I would like to use either Symbolics.jl or Sympy.jl, but I don't see them listed.
4. Do I submit a PR when I start with the lecture and then continue working with the branch till you are satisfied with the result, or do I complete the entire lecture and then submit a PR?
5. I see that there are some unit tests in some of the Julia lectures. Is this something that I should do for all code snippets when possible? I am not that familiar with these types of tests, but they seem easy enough to implement. Any direction here would help.

Sorry for all the questions, just want to make sure I get it right the first time.

Dawie

[jlperla]

@mmcky Hi matt, I noticed when I clicked on the "deploy_preview" I can't see the CI results? Is there a bug somewhere in the CI/CD process? Since @DawievLill made the PR from his own fork, would that change anything? Thanks!

[jlperla]

Thanks @DawievLill !

Funny enough, this is the one that I had started messing with in QuantEcon/lecture-source-jl#844 with the old build system, but stopped since we were moving to the jupyterbook and because I wanted to wait for Symbolics/MTK/etc.. You can check that for some code to see if it is helpful though (but don't copy the text necessarily).

Have I created the branch correctly to be merged with the main repository? Or have I made some mistake?

Good enough! We will check before anything gets merged anyways.

Do I have to execute the Jupyter Book locally before I submit a PR, or can I only make changes to the markdown file and submit.

You will definetely want to make sure that the whole jupyterbook builds for the PR, and that you run it before saying it is complete. I know that there are some issues right now which prevent #84 from being turned on, so we need to be especially careful to check the jupyterbook output locally to look for errors and warnings then. We need to make sure it is warning free before being merged.

What happens if I want to introduce a package that is not currently listed in the project .toml? For example, for this lecture I would like to use either Symbolics.jl or Sympy.jl, but I don't see them listed.

In general, we want to avoid adding packages whenever possible since the changes in the manifest can cascade and cause trouble on the installation. SymPy.jl is a no-go since managing python dependencies is a nightmare. Symbolics.jl is fine since I am planning to add it in the next year anyways.

So what you should do is (making sure that you always work with an activated project such as https://github.com/QuantEcon/lecture-julia.myst#executing-code-in-markdown-files when you write your own stuff) is
] add Symbolics and then make sure the update to the Manifest and Project files are part of the PR. Do the bare minimum of package changes though to avoid breaking other lectures.

Do I submit a PR when I start with the lecture and then continue working with the branch till you are satisfied with the result, or do I complete the entire lecture and then submit a PR?

Totally up to you. I think creating a PR early with the "WIP" in the name is helpful since it lets you use the cool vscode tricks for communication. I can make a comment on a line of code, you can modify it or respond on it, etc.

I see that there are some unit tests in some of the Julia lectures. Is this something that I should do for all code snippets when possible? I am not that familiar with these types of tests, but they seem easy enough to implement. Any direction here would help.

The unit tests were especially important at one point when making the code work from Julia 0.6 to 1.0. They are a little less important now to be too extreme, but when we upgrade Julia or if people want to tweak any code, then they will help us make sure nothing broke. It doesn't need to be like the unit tests for a proper software package. So I would:

• throw in a couple of checks on numbers coming out of core calculations, just so that if we go back and make changes to those calculations to "improve them" we will know if things obvious break.
• How many checks to add depends on the complexity and type of a function, but often just checking the end result of a long sequence of calculations is good enough. If there is an error, we could go back to see why it happened, and we can deal with the possibility that two errors in the code may have canceled out for that exact check.
• Never check numbers that come out of random variables or simulations, since seeds are a mess

The most important thing, though, is to stick with the coding style and conventions used throughout the lectures. Style of naming variables, use of named tuples and avoiding structs or type annoations, writing generic code wheenver possible, etc. See https://github.com/QuantEcon/lecture-julia.myst/blob/main/style.md for some notes on this, but things may have evolved a bit.

And finally, I would make sure not to change any of the actual text of the lectures except for trivial stuff related to which packages it uses. But if you have issues then I am happy to take a look.

[DawievLill]

Thanks for taking the time to give detailed answers @jlperla , it helps a lot.

It is always tricky at first to get used to other workflows and styles. However, your comments are quite clear and it definitely helps to facilitate the transition. I am sure I will still make mistakes, but I guess that's par for the course.

[mmcky]

@mmcky Hi matt, I noticed when I clicked on the "deploy_preview" I can't see the CI results? Is there a bug somewhere in the CI/CD process? Since @DawievLill made the PR from his own fork, would that change anything? Thanks!

@jlperla indeed! Sadly the github actions don't function the same way from forks (due to different trust). I haven't seen a good solution for this but it's not good :-(. It is one of the reasons the main branch doesn't auto publish so it can be used as a staging ground.

Actually just seen this new documentation: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

I'll give this a try -- but docs don't seem to match our menus on this.

[mmcky]

@jlperla @DawievLill #128 may help get this running.

@DawievLill generally don't recommend making any changes to .github/workflows/ci.yml but it is a very small change so happy for you to add this if you wish. Otherwise @jlperla we can merge #128 and @DawievLill can fetch the latest upstream.

[mmcky]

@DawievLill I merged in #128 to see if we can get ci workflows running from your fork.

If you want to fetch the latest upstream changes then I can see if we get the approval screen to run the workflows which provides preview and does execution checks etc.

[DawievLill]

@mmcky I fetched the upstream changes and made one commit. Let me know if there is anything else that I should do.

Thanks for the links to the Github docs, reading through everything to make sure I understand all the moving parts.

[mmcky]

Thanks @DawievLill the Approve and Run box showed up -- so I have enabled the github actions to run so you should get a preview build soon.

[DawievLill]

Thanks @mmcky . The build seems to fail because of LaTeX dependencies not installing properly. I don't think this is something that I can fix from my side? The build executes fine when I run it locally.

[mmcky]

shoot - it looks like repo secrets still aren't accessible so we get

Netlify credentials not provided, not deployable

as part of the github action run.

Update: Secrets are not passed to workflows that are triggered by a pull request from a fork -- Makes sense but bummer. Can't get the automatic netlify deploy for the preview. @jlperla the _build folder is attached to the workflow you could download and open _build/html if you wanted to preview -- but I guess the best way forward is once you're happy with the source you can merge to master and see the preview there before tagging a live release.

[DawievLill]

@jlperla With the latest commit I added a first attempt at a 3d graph that uses the GR backend. The camera angle is tricky to get right. I also constructed a small meshgrid function to align things more closely to the Python lectures. Normally I would use ndgrid from LazyGrids.jl, but I don't want to introduce new packages.

I am going to do the symbolic math part next, which should hopefully work without a hitch. After that I think the lecture is almost done for review. I am in no rush, just giving an update.

[jlperla]

Thanks @DawievLill Yeah, lets avoid LzygGrids.jl since it isn't a major package and we need to be careful about having dependencies which might end up unmaintained.

[DawievLill]

@jlperla First draft of the lecture is done. It contains all the figures and calculations. I tried to be careful to read through it all, but there might still be some mistakes. Important to note that I included the Symbolics.jl package.

Enjoy your weekend!

[jlperla]

Thanks @DawievLill Looks grea.t I put in a few quetsions, but after that I can have @jbrightuniverse look to merge it. We will need to be a little careful with the package addition, but normally it is a relatively easy step to merge.

[jlperla]

@jbrightuniverse I think this is ready for you to do a sanity check on and then merge if you are happy with it. The only thing that concerns me is the bump on the linear algebra depenencies. The libblastrampoline thing is Julia 1.7+ which isn't yet released. I don't know if that could break usage on 1.6, but it isn't work the risk.

So I would feel little bit more comfortable if we got rid of the changes to the manifest/project from this PR and did the ] add Symbolics to change the Project.toml file on a Julia 1.6 setup.

Can you guys coordinate on that? It might be trickier than normal since it is in the fork from @DawievLill . One solution is for him to just throw out the changes to the project/manifest from this PR, then you could quikcly merge it (knowing that CI will break) and then update the project.toml file separately with the ] add Symbolics. Then we can check the final result before tagging new reelease.

@DawievLill I am going to make you a collaborator on these lectures so that you can make branches and PRs directly here. It will be a little easier to edit each others PRs then.

[DawievLill]

@jlperla and @jbrightuniverse How would I go about removing the changes to the project and manifest files on my side?

[jbrightuniverse]

@DawievLill for removing the changes, you could try reverting the commit where you made the changes (or if that's too far in the commit history, take the versions we have on the repo and upload them to your version which should roll back anything different).

[DawievLill]

Thanks @jbrightuniverse, I think reverting would take it too far back. Always afraid of messing up the process with reverts. I will copy the files over and make a new commit.

The only snag might be the introduction of the Symbolics package. I couldn't pick up from the previous comments whether it is included in the TOML files for the main repo.

[jbrightuniverse]

A quick ctrl-F of the manifest shows Symbolics.jl is already there, but it is not in Project.toml so it will need to be added to it under a 1.6 environment.

[jlperla]

Perfect. In that case the Project.toml change should be fine and we can just get rid of any manifest changes

[DawievLill]

@jbrightuniverse Ok, I have cleared out the changes to the Project and Manifest files with the latest commit.