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

Run notebooks once a week in a separate workflow #263

Merged
merged 14 commits into from
Feb 18, 2022

Conversation

ismael-mendoza
Copy link
Collaborator

@ismael-mendoza ismael-mendoza commented Feb 16, 2022

This page was useful for how to setup the cron job: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule.

I set the cron job to run every Monday at 7a EST but I'm open to suggestions on this

@ismael-mendoza
Copy link
Collaborator Author

ismael-mendoza commented Feb 16, 2022

@thuiop - I've been wandering about this for a while. Should we bother checking the output of the cells of the notebook and checking if they are the same with sanitize? I think I would be satisfied with just checking if any of the cells failed to run.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #263 (6e0f745) into main (ce447d9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #263   +/-   ##
=======================================
  Coverage   88.26%   88.26%           
=======================================
  Files          13       13           
  Lines        1526     1526           
=======================================
  Hits         1347     1347           
  Misses        179      179           
Flag Coverage Δ
unittests 88.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce447d9...6e0f745. Read the comment docs.

@ismael-mendoza
Copy link
Collaborator Author

I would suggest this flag

-nbval-lax, which runs notebooks and checks for errors, but only compares the outputs of cells with a #NBVAL_CHECK_OUTPUT marker comment.

if there is a output that is really important is the same then we can add that #NBVAL_CHECK_OUTPUT to that cell.

@ismael-mendoza
Copy link
Collaborator Author

ismael-mendoza commented Feb 17, 2022

I realized that if we want to check outputs are exactly the same that should be a test anyways so I removed the sanitize.cfg and switched to pytest --nbval-lax notebooks.

@thuiop
Copy link
Collaborator

thuiop commented Feb 17, 2022

@thuiop - I've been wandering about this for a while. Should we bother checking the output of the cells of the notebook and checking if they are the same with sanitize? I think I would be satisfied with just checking if any of the cells failed to run.

I'm not sure. I agree that it may be sufficient ; checking the output value ensures that it is "exactly the same" as it should be, but I am not entirely sure this is useful.

@ismael-mendoza
Copy link
Collaborator Author

Thanks @thuiop I think if we really want to check a function outputs exactly the same output this is better off as a test. The only thing about notebooks is that there are images but those we need to look at them by eye anyways

@thuiop
Copy link
Collaborator

thuiop commented Feb 17, 2022

It looks good to me but actually I wonder whether this is not a bit too far. I think we should check that the notebooks are actually runnable before merging to main ; maybe there is a way so that it does not do it automatically but there is a button we can click on before merging.

@ismael-mendoza
Copy link
Collaborator Author

ismael-mendoza commented Feb 17, 2022

In my terminal I ran it w/ both nbval and nbval-lax and both of them pass so I think we should be ok ?

Could you remind me why are the last 3 cells of 00-intro.ipynb skipped ? (and how you set them up so they would be skipped?)

@ismael-mendoza
Copy link
Collaborator Author

ismael-mendoza commented Feb 17, 2022

It looks good to me but actually I wonder whether this is not a bit too far.

I understand the concern, if you think it is useful to check output is exactly the same (maybe you have some notebook/cell in mind?) we can revert this last change.

In my experience a notebook not having exatly the same output is usually not a sign of an error (hopefully the tests would catch that!) like when we added the progress bar in your other PR.

Incidentally, how would you fix this last error? Where it is ok that the notebook cell changed with the progress bar? Would you need to sanitize it away?

@ismael-mendoza
Copy link
Collaborator Author

I can also see having an alert once a week notifying us that a notebook cell changed (even if small) might be good to review so I'm open to keeping it

@ismael-mendoza
Copy link
Collaborator Author

After more thought and learning about nbval a bit more, I decided to keep the sanitize as is

@ismael-mendoza ismael-mendoza merged commit 548f36e into main Feb 18, 2022
@ismael-mendoza ismael-mendoza deleted the run-notebooks-once-a-week branch February 18, 2022 13:06
@ismael-mendoza
Copy link
Collaborator Author

nbval passing in my terminal so I'm merging this now

ismael-mendoza added a commit that referenced this pull request Feb 25, 2022
* Simplify CLI with hydra instantiate (#230)

* update poetry

* more error checking in draw_blends for MR

* bug in this line when using basic measure function and MR

* catch error related to meas_band_num in metrics

* use hydra instantiate and call rather than available_*

* update documentation

* test MR on main, remove error checking now done by hydra

* Revert "update poetry"

This reverts commit aad74ea.

* Custom measurement functions (#231)

* link the CLI tutorial

* clarify that custom survey, measurement functions from CLI are not supported.

* add need blank line

* clarify how to add measure function if you are advanced user

* fix url (#232)

* fixing test

* make test always predictable

* pytest information in pyproject toml (#233)

* Update CONTRIBUTING.md

* More reproducibility (#237)

* only able to provide seed not rng

* only allow integer seeds, propagate to multiprocessing correctly

* propagate changes to tests

* correct seed defaults

* fix tests bc seeds were not propagated correctly before

* larger max int

* Update btk/draw_blends.py

Co-authored-by: Alexandre Boucaud <[email protected]>

* Update btk/sampling_functions.py

Co-authored-by: Alexandre Boucaud <[email protected]>

* define default seed once

* Update btk/sampling_functions.py

Co-authored-by: Alexandre Boucaud <[email protected]>

* dont need another rng embedded in render_blend

* simplify line

* seeds are now used in config

* put defualt seed in init

* show how to use the seed

* add seed in tutorial documentation too

* ignore warning until actually throws an error

* fix tests with new seed

* why did the precision decrease again?

* no output in first cell

Co-authored-by: Alexandre Boucaud <[email protected]>

* Small fixes to the measure file (#242)

* Small fixes to the measure file

* Fixed pixel and world coordinates

* Fixed notebooks

* Small improvements tutorials (#243)

* Renamed notebooks and added readme

* Save in temporary directory in intro notebook

* update name of ntoebooks in test.yml

* remove mentions of coadd

* warning about jupyterlab

Co-authored-by: Ismael Mendoza <[email protected]>

* Invisible shifts and indices (#244)

* add notes to documentation for users about shifts/indexes

* remove shifts/ indices from notebooks to avoid confusing users

* Update CONTRIBUTING.md (#245)

* Update CONTRIBUTING.md

* Update CONTRIBUTING.md

* trailing white space

* Ensure PSF is consistently propagated if customized (#246)

* should be a public method

* remove mention of more examples that are non existant

* fix urls (#247)

* Fixed noise seed identical over a batch (#249)

* Fixed noise seed identical over a batch

* Rework seed management

* Documentation for seeds

* Changed behaviour to something more logical

* Moved seed sequence initialization and renamed variables

* remove galsim hub and tensorflow temporarily (#251)

* remove galsim hub and tensorflow

* forgot mention in --extras

* Update notebooks/README.md

Co-authored-by: thuiop <[email protected]>

Co-authored-by: thuiop <[email protected]>

* No capping (#252)

* no capping, suppress warnings

* update poetry

* fix tests after updating

* Separate noises (#254)

* Separate the two noises

* Updated tests

* Possibility to choose which noise is drawn

* Update doc and notebook

* Changed lists to tuples

* Fix test

* Fix all tests

* Updated notebooks

* Revert change

* Notebooks

* Bump ipython from 7.31.0 to 7.31.1 (#257)

Bumps [ipython](https://github.com/ipython/ipython) from 7.31.0 to 7.31.1.
- [Release notes](https://github.com/ipython/ipython/releases)
- [Commits](ipython/ipython@7.31.0...7.31.1)

---
updated-dependencies:
- dependency-name: ipython
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fetch flake8 from GitHub instead of GitLab (#260)

Co-authored-by: Alexandre Boucaud <[email protected]>

* update poetry adn pyproject w/ tqdm

* fixing problem with add_noise (#264)

* Run notebooks once a week in a separate workflow (#263)

* Add progress bar

* add description

* new cron job for notebooks

* update other workflows

* remove push

* forgot to remove notebooks in test.yml

* use nbval-lax

* clarify comment in new job

* not sure why tqdm change was here

* undo sanitize change

* ignore nbval warning

* fix warning

Co-authored-by: thuiop <[email protected]>

* Add progress bar (#258)

* Add progress bar

* add description

* update poetry

* add process to progress bar description

* get current process function

* sanitize progress bar

* run notebooks with progress bar, removed autoreload syntax

Co-authored-by: Ismael Mendoza <[email protected]>

* Parametric cosmos simulations (#256)

* parametric sims with cosmos cat

* updated docstring

* tutorial with parametric example

* adding exclusion_level cuts

* pre-commit changes

* resolve pre-commit changes

* using catalog directly from galsim

* Update 01a-cosmos_tutorial.ipynb

fixing kernel metadata

* fixing kernel metadata

* updating docstring

* default exclusion_level as "marginal"

* _selection.fits catalog

* correcting path

* added tests with parametric sims

* updated config

* removed assert on exclusion_level

* updated add_noise

Co-authored-by: thuiop <[email protected]>
Co-authored-by: Alexandre Boucaud <[email protected]>
Co-authored-by: Ismael Mendoza <[email protected]>

* keep notebook output but remove metadata with nbstripout (#266)

* keep output but remove metadata

* add nbstripout to pre-commit

* add nbstripout to dependencies

* keep output nbstripout

* error corrected

* autoupdate

* update poetry

* black

* black should be latest version

* add notebook action badge (#267)

Co-authored-by: Alexandre Boucaud <[email protected]>
Co-authored-by: thuiop <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Biswajit Biswas <[email protected]>
ismael-mendoza added a commit that referenced this pull request Feb 25, 2022
* Add progress bar

* add description

* new cron job for notebooks

* update other workflows

* remove push

* forgot to remove notebooks in test.yml

* use nbval-lax

* clarify comment in new job

* not sure why tqdm change was here

* undo sanitize change

* ignore nbval warning

* fix warning

Co-authored-by: thuiop <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants