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

79 add nbmake or equivalent pytest extension for notebook e2e tests #87

Conversation

aidanjgriffiths
Copy link
Collaborator

A new score or metric should be developed on a separate feature branch, rebased against the main branch. Each merge request should include:

The implementation of the new metric or score in xarray, ideally with support for pandas and dask
100% unit test coverage
A tutorial notebook showcasing the use of that metric or score, ideally based on the standard sample data
API documentation (docstrings) using Napoleon (google) style, making sure to clearly explain the use of the metrics
A reference to the paper which described the metrics, added to the API documentation
For metrics which do not have a paper reference, an online source or reference should be provided
For metrics which are still under development or which have not yet had an academic publication, they will be placed in a holding area within the API until the method has been properly published and peer reviewed (i.e. scores.emerging). The 'emerging' area of the API is subject to rapid change, still of sufficient community interest to include, similar to a 'preprint' of a score or metric.
Add your score to summary_table_of_scores.md in the documentation

All merge requests should comply with the coding standards outlined in this document. Merge requests will undergo both a code review and a science review. The code review will focus on coding style, performance and test coverage. The science review will focus on the mathematical correctness of the implementation and the suitability of the method for inclusion within 'scores'.

A github ticket should be created explaining the metric which is being implemented and why it is useful.

@aidanjgriffiths aidanjgriffiths self-assigned this Nov 1, 2023
@aidanjgriffiths aidanjgriffiths linked an issue Nov 1, 2023 that may be closed by this pull request
@aidanjgriffiths aidanjgriffiths force-pushed the 79-add-nbmake-or-equivalent-pytest-extension-for-notebook-e2e-tests branch from 13e1045 to 911ab5d Compare November 1, 2023 05:32
@aidanjgriffiths
Copy link
Collaborator Author

@John-Sharples and @tennlee, here is the notebook stage, note I needed to amend the pyproject.toml tool.pytest.ini_config as the ignore addopts wouldn't work in list format. I have a suspicion that pytest only handles a string value for addopts. Its a bit messy but its the only way to have non-failing test run when running in the root. Let me know what you think.

@aidanjgriffiths aidanjgriffiths force-pushed the 79-add-nbmake-or-equivalent-pytest-extension-for-notebook-e2e-tests branch 2 times, most recently from b6418bb to 1b6a782 Compare November 1, 2023 23:30
@aidanjgriffiths
Copy link
Collaborator Author

aidanjgriffiths commented Nov 2, 2023

this comment is no longer relevant. I found that the issue was with the extra quotations needed because of the spaces in the notebook name. I've fixed it by renaming the notebooks to use capitalised snake_case and this works well. I suggest all files should be named without spaces to ensure compatibility with linux tools.

This is ready to review with this mornings conflict resolved.

@aidanjgriffiths aidanjgriffiths marked this pull request as draft November 2, 2023 03:08
@aidanjgriffiths aidanjgriffiths force-pushed the 79-add-nbmake-or-equivalent-pytest-extension-for-notebook-e2e-tests branch 17 times, most recently from 27997e1 to a6d79f8 Compare November 2, 2023 05:39
@aidanjgriffiths aidanjgriffiths marked this pull request as ready for review November 2, 2023 05:42
@aidanjgriffiths aidanjgriffiths force-pushed the 79-add-nbmake-or-equivalent-pytest-extension-for-notebook-e2e-tests branch from a6d79f8 to 7325bdb Compare November 2, 2023 05:43
@aidanjgriffiths
Copy link
Collaborator Author

aidanjgriffiths commented Nov 2, 2023

@tennlee, Ready for review, turned out the coverage through a non editable install fails to return any coverage as the code it runs off is in a different location that the repository in the runner. Adding pip install -e . in the pytest stage fixes. Very frustrating one to debug. At least now the pipeline will fail when coverage dips below 99, currently at 99.56% with branch coverage enabled.

EDIT: I just pushed a419bd0 which remove stages on push, which duplicates the stages when also running on a pull request. Since all code that goes into develop and main is from a PR I think its safe to disable this as it'll save pipeline time.

@tennlee tennlee merged commit d779c1a into develop Nov 2, 2023
6 checks passed
@tennlee tennlee deleted the 79-add-nbmake-or-equivalent-pytest-extension-for-notebook-e2e-tests branch May 9, 2024 12:42
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.

Add nbmake or equivalent pytest extension for notebook e2e tests
2 participants