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

integrate smarteole example into ci #45

Merged
merged 5 commits into from
Jan 21, 2025
Merged

integrate smarteole example into ci #45

merged 5 commits into from
Jan 21, 2025

Conversation

samuelwnaylor
Copy link
Collaborator

@samuelwnaylor samuelwnaylor commented Jan 20, 2025

About the change

The examples/smarteole_example.py is an essential test to ensure wind-up gets the intended 'correct' answers. Before this PR, there was already a check_results=True argument that could be passed to the print_smarteole_results function in order to assert the results are as expected. This PR, integrates this essential test into the pytest test suite of wind-up. This also means that the smarteole example will be run in github actions, which removes the need to manually run examples/smarteole_example.py that has been done historically when checking PRs.

Related issue

Closes #21

Notes

  • Uses GitHub Large File Storage (LFS) to handle the smarteole data in the tests/test_data/smarteole directory. Only the files that are used in the smarteole example are stored from the original Zenodo dataset.
  • In order to avoid the slow running tests whilst developing locally, I have added a poe task, test-fast, that avoids tests marked as slow (currently only smarteole).

@samuelwnaylor samuelwnaylor force-pushed the lfs branch 10 times, most recently from 6ba621d to 1fa3ccf Compare January 20, 2025 13:44
@samuelwnaylor samuelwnaylor changed the title Lfs integrate smarteole example into ci Jan 20, 2025
gabrielecalvo
gabrielecalvo previously approved these changes Jan 21, 2025
Copy link
Collaborator

@gabrielecalvo gabrielecalvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good 👍

Copy link
Contributor

@aclerc aclerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

@@ -44,6 +44,8 @@ Use `poe all` to run all required pre-push commands (make sure the virtual envir
## Running tests
Install dev dependencies and use `poe test` to run unit tests (make sure the virtual environment is activated)

For convenience when developing locally, run `poe test-fast` to avoid running the tests marked as slow.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks for adding this!

)
smarteole_data = SmarteoleData(scada_df=scada_df, metadata_df=metadata_df, toggle_df=toggle_df)

main_smarteole_analysis(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls assert_frame_equal internally. Seems fine to me. Maybe worth bubbling up a boolean output so an explicit assert is shown in the test, but I'm not sure if that's an improvement.

@aclerc aclerc merged commit c50878d into main Jan 21, 2025
2 checks passed
@aclerc aclerc deleted the lfs branch January 21, 2025 17:34
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.

run smarteole_example.py in CI
3 participants