alan-turing-institute / DTBase

A starting point from which digital twins can be developed.
MIT License
11 stars 4 forks source link

Create BaseService and incorporate into Ingress and Models #190

Closed EdwinB12 closed 6 months ago

EdwinB12 commented 7 months ago

Linked to #184 #181 #81

Tasks:

Compulsory things to do

Extras

codecov-commenter commented 7 months ago

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (a202d4b) 89.01% compared to head (ba40cca) 89.14%. Report is 2 commits behind head on main.

:exclamation: Current head ba40cca differs from pull request most recent head 944e303. Consider uploading reports for the commit 944e303 to get more accurate results

Files Patch % Lines
dtbase/models/arima.py 85.40% 27 Missing :warning:
dtbase/ingress/ingress_weather.py 70.83% 7 Missing :warning:
dtbase/tests/generate_synthetic_data.py 36.36% 7 Missing :warning:
dtbase/services/base.py 92.85% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #190 +/- ## ========================================== + Coverage 89.01% 89.14% +0.12% ========================================== Files 77 76 -1 Lines 5163 5304 +141 ========================================== + Hits 4596 4728 +132 - Misses 567 576 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

EdwinB12 commented 7 months ago

Remaining jobs on this PR

review-notebook-app[bot] commented 6 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

EdwinB12 commented 6 months ago

@GiorgioCerro @mhauru

The main things to check in this PR are:

Thanks!

EdwinB12 commented 6 months ago

Thanks for the review @mhauru , I've addressed your comments.

Now, the tests are failing. I wonder if its not worth testing for exact Arima results in the tests? The tests didn't before this and its just causing tests to fall over on 3rd decimal places. .

mhauru commented 6 months ago

Thanks Edwin, I'll try to take a look later today.

Now, the tests are failing. I wonder if its not worth testing for exact Arima results in the tests? The tests didn't before this and its just causing tests to fall over on 3rd decimal places.

Yeah, if it's turning into a pain then we can give this up. Maybe we can do some other sanity checks of the results instead (or do other tests do that already?). You could also reaaally reduce the precision on the checks, but I don't know if we have a guarantee that that won't ever fail, and indeterministically failing tests are nasty.

mhauru commented 6 months ago

I tried running the Azure function locally, had to make this change: #213 but with that it all works nicely.

EdwinB12 commented 6 months ago

Changes made, just need an approval @mhauru

EdwinB12 commented 6 months ago

closes #184