test_load_corrector #162
test_load_corrector #162
Conversation
NormannK
commented
Oct 10, 2024
NormannK
commented
- added a test for class_load_corrector.py
- added type hints and comments to the class_load_corrector.py
|
@NormannK Please resolve the confilicts Thanks |
* Dockerfile: Use non-root user, buildx cache, setup for readonly container, remove unused apt deps. For now don't install pip package and keep development flask server as this will be replaced in the future (fastapi). Then a proper webserver (e.g. nginx) should be used and the pip package can be created and deployed just to the run-stage (with the webserver). * docker-compose: Set to readonly (anonymous volumes declared in Dockerfile should maintain all writable data). Mount config.py for easier development. Should be replaced by environment support for all config file variables. * Remove unused runtime dependencies: mariadb, joblib, pytest, pytest-cov. * Move pytest-cov to dev dependencies. * Add output_dir to config.py. * Fix visualization_results.pdf endpoint. * Update docs.
|
Please re-run all tests |
|
done |
There was a problem hiding this comment.
Testing is good, but we should be careful to avoid locking down the codebase to the current architecture and making it harder to do important refactoring in the future.
For example, here are two things that if addressed could make things easier in the long run:
- The
LoadPredictionAdjusteris extremely stateful. The relevant data are stored as attributes that are set or modified by calling the methods in exactly the correct order. A scikit-learn Pipeline might be more suited for this sequence of data transformations. - The
LoadPredictionAdjusteris initialized with this magicload_forecast: object, but it doesn't actually need theload_forecast! All it needs is a callable with the same signature asget_daily_statswhich can be type annotated as aCallable. Then the test also wouldn't need aMagicMock.
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
Co-authored-by: Michael Osthege <[email protected]>
|
thanks for you thorough review. I don't know what happen with those double lines. |
Maybe address 2. in this PR because it simplifies the test? I will propose 1. in a discussion together with some other proposals along the same line. 👉 #187 |
