Add notebooks to GitHub actions

[mj-gomes]

Hi,

As discussed in the last meeting, we need to put the notebooks as part of the CI tests. This PR creates a new separate workflow for notebook tests, using nbmake. I added it as a separate workflow because it can be slow, since notebooks often need large enough simulations to be able to show maps etc. This way, it runs in parallel with the other tests.

For commits where we want to skip the notebook tests, we can add [skipnb] or [skip nb] to the commit name.

I also fixed somes bugs in the notebooks that were leading to fails due to some lack of updates on the notebooks with the recent changes in lbs.

Please note:

1. we should probably now add assertions to each notebook; right now the tests only see if the notebooks yield some error or not, therefore not checking their physical correctness.
2. the changes done in hwp_harmonics in this PR should be temporary. I realized when fixing the errors in the notebooks that there is something weird on that module when using galactic coordinates instead of ecliptic. I am on it. What I did now made the notebooks work but also made some of the hwp tests fail. I hope I find the reason to this in the next days and fix this. -> (being solved in fix psi computation in ecliptic->galactic pointing rotation #543)

After these two things, I believe this PR will be ready to go. For now, I leave it as a draft.

[ggalloni]

Hi @mj-gomes, thanks for initiating this!

As a matter of fact, these days I did something similar to test the notebooks on another repo, so I might be able to help with this 👍

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
litebird_sim
coordinates.py					183-215
Project Total

This report was generated by python-coverage-comment-action

[anand-avinash]

we should probably now add assertions to each notebook; right now the tests only see if the notebooks yield some error or not, therefore not checking their physical correctness.

Do we really need this, as all the tests in test/ perform exactly the same task of checking the physical correctness?

Another suggestion: It will be useful to provide a script similar to run_tests.sh that allows anyone to run all the tests implemented for github workflow, in their local environment. Even better, if we run these test scripts in workflow. This will allow anyone to test the codebase in a consistent manner before pushing changes to remote. This script may include simple pytest without MPI, pytest with MPI, doctests, code style test, notebook tests, etc.

[mj-gomes]

Hi @anand-avinash

Do we really need this, as all the tests in test/ perform exactly the same task of checking the physical correctness?

I can imagine some cases in which this would be helpful to force us to not forget to update the notebooks. A change in lbs may not necessarily result in an error in a notebook, but still change its physical correctness in cases that break backward compatibility. For example, recently I changed the band integration scanning so that it returns the TOD normalized to the bandwidth, while before it was not the case. The notebooks will now have different results, which may not make sense or be properly explained in there. Having a simple test that checks that the output tod is equal to an expected tod, means that with some change like this one, we'll get an error if we forget to update the notebook. What I am trying to say is that these simple tests would not be a way to test lbs functionalities, as that already exists as you said, instead they would be a way to ensure we don't forget updating notebooks, as it has been happening until now.

Another suggestion: It will be useful to provide a script similar to run_tests.sh that allows anyone to run all the tests implemented for github workflow, in their local environment. Even better, if we run these test scripts in workflow. This will allow anyone to test the codebase in a consistent manner before pushing changes to remote. This script may include simple pytest without MPI, pytest with MPI, doctests, code style test, notebook tests, etc.

Agree!

[anand-avinash]

What I am trying to say is that these simple tests would not be a way to test lbs functionalities, as that already exists as you said, instead they would be a way to ensure we don't forget updating notebooks, as it has been happening until now.

This makes sense, Thanks!

[ggalloni]

Hi @mj-gomes and @anand-avinash, as regards the assertions (or similar) in notebooks, I am not sure I agree.

I think that actual CI tests should ensure full correctness. So, if we find out something that should be tested while running notebooks, we should create a new dedicated test instead of relying on the notebook.

Historically, notebooks have been drifting away due to API changes that we forgot to port, and this will be caught by CI running notebooks. For the rest, I think they should remain pure functionality showcases.

[mj-gomes]

Hi @mj-gomes and @anand-avinash, as regards the assertions (or similar) in notebooks, I am not sure I agree.

I think that actual CI tests should ensure full correctness. So, if we find out something that should be tested while running notebooks, we should create a new dedicated test instead of relying on the notebook.

Historically, notebooks have been drifting away due to API changes that we forgot to port, and this will be caught by CI running notebooks. For the rest, I think they should remain pure functionality showcases.

I see your point. In that case, this PR is already doing what's need. The docs errors come from the changes in #543, because I merged them in here. We have to fix those before finishing this PR.