Skip to content

Refactor weather app to use basic DataManager and Config objects#44

Merged
mmaelicke merged 25 commits intomainfrom
refactor-weather-app
Mar 16, 2022
Merged

Refactor weather app to use basic DataManager and Config objects#44
mmaelicke merged 25 commits intomainfrom
refactor-weather-app

Conversation

@mmaelicke
Copy link
Member

With this PR in place, the new and highly important DataManager is used throughout the weather app. This should prioritize singleton usage throughout the whole streamlit instance. This should really speed things up, but make user uploads a hell more complicated, which we don't use anyway.

@AlexDo1 you could checkout this branch and run the state of this branch to report issues and errors back. These errors should then be resolved with the next release.

@AlexDo1
Copy link
Collaborator

AlexDo1 commented Mar 16, 2022

For some reason the tests are no longer running on Github, what did I do wrong?
On my laptop now only the topic_selector test does not run successfully and I'm not sure how to do that either, because I would have to read out the session_state in the test (?), I don't know how to do that.
Can you take over @mmaelicke ?

@mmaelicke
Copy link
Member Author

Ah, yeah. I broke that.

@AlexDo1
Copy link
Collaborator

AlexDo1 commented Mar 16, 2022

Hm there's the old xarray IO backend error again.
Locally on my machine the weather test works..

@mmaelicke
Copy link
Member Author

I removed the topic_selector tests altogether. I can't find any working way to mock a session state. Without that, unittesting anything relying on the state does just not work.

@mmaelicke
Copy link
Member Author

Hm there's the old xarray IO backend error again. Locally on my machine the weather test works..

Yeah, found the issue. There was a test that used the default config and not the test config. Thus, it was trying to load the original data, not the test data, which is not available in GH actions. Should be fixed now.

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #44 (9903dad) into main (acf5830) will increase coverage by 2.44%.
The diff coverage is 30.12%.

@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
+ Coverage   17.90%   20.34%   +2.44%     
==========================================
  Files          14       15       +1     
  Lines        1229     1322      +93     
==========================================
+ Hits          220      269      +49     
- Misses       1009     1053      +44     
Impacted Files Coverage Δ
ruins/plotting/kde.py 4.00% <0.00%> (ø)
ruins/apps/weather.py 34.89% <11.59%> (-0.21%) ⬇️
ruins/components/topic_select.py 33.33% <20.00%> (-66.67%) ⬇️
ruins/plotting/weather_data.py 8.10% <33.33%> (+1.25%) ⬆️
ruins/plotting/maps.py 13.43% <41.66%> (+5.85%) ⬆️
ruins/core/build.py 36.84% <50.00%> (+36.84%) ⬆️
ruins/core/cache.py 50.00% <50.00%> (ø)
ruins/core/config.py 84.12% <70.00%> (-13.10%) ⬇️
ruins/core/data_manager.py 80.34% <84.61%> (-0.74%) ⬇️

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 acf5830...9903dad. Read the comment docs.

@mmaelicke
Copy link
Member Author

Nice, unittest are running now. I will merge

@mmaelicke mmaelicke merged commit 564950d into main Mar 16, 2022
@mmaelicke mmaelicke deleted the refactor-weather-app branch March 16, 2022 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

Inject the new data-api into the app map the hdf5 files into the data API

2 participants