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

I359 programmatically query results #367

Merged
merged 30 commits into from
May 7, 2019

Conversation

fcooper8472
Copy link
Contributor

See #350.

Current shortcomings are:

  • Can only handle a single output being requested at a time
  • Testing is inadequate: can't yet figure out how to mock a working store that includes all the components necessary to test the get_results() method.

@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

Merging #367 into develop will increase coverage by 0.29%.
The diff coverage is 83.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #367      +/-   ##
===========================================
+ Coverage    70.73%   71.02%   +0.29%     
===========================================
  Files           59       60       +1     
  Lines         5204     5336     +132     
  Branches       618      659      +41     
===========================================
+ Hits          3681     3790     +109     
- Misses        1431     1449      +18     
- Partials        92       97       +5
Flag Coverage Δ
#javascript 71.02% <83.09%> (+0.29%) ⬆️
#python 78.73% <83.09%> (+0.11%) ⬆️
Impacted Files Coverage Δ
src/smif/data_layer/file/file_data_store.py 87.07% <100%> (ø) ⬆️
src/smif/data_layer/store.py 90.51% <76.82%> (-4.76%) ⬇️
src/smif/data_layer/results.py 91.52% <91.52%> (ø)
src/smif/data_layer/data_array.py 86.89% <0%> (+0.68%) ⬆️

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 a5d2e47...b94bce9. Read the comment docs.

@willu47
Copy link
Member

willu47 commented May 2, 2019

I'm happy with the functionality now. @tomalrussell wanted to have a look through the testing options for you @fcooper8472, so over to him.

@fcooper8472
Copy link
Contributor Author

@tomalrussell wanted to have a look through the testing options for you @fcooper8472, so over to him.

That would be really helpful - I've been struggling to mock a "complete" state that includes the necessary model runs, sector models and results. If we could work out how to provide that in a fixture it would be useful more widely, but would definitely allow us to thoroughly test this.

Include style niggles:
- docstring parameter description on next line
- prefer raising relevant errors to assertions
- avoid abbrev.s in param. names (sec_model_name > model_name)
Copy link
Member

@tomalrussell tomalrussell left a comment

Choose a reason for hiding this comment

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

This looks good to merge - especially the key Results.read method seems to hit the spot.

If the testing around Results.read and Store.get_results can be fleshed out reasonably straightforwardly, based on the results_with_results fixture, that would be great, otherwise let's not block this PR.

Looking forward to using this in coming weeks! 👍

@fcooper8472
Copy link
Contributor Author

I'm in the process of tinkering with the fixtures to improve the test coverage - but happy for you to merge this now and look at further changes down the line if you prefer?

@tomalrussell
Copy link
Member

I'll pick this up on Tuesday afternoon and aim to cut 1.1 (#369) - a bit of tinkering and coverage would be welcome if you can fit it in before then.

@codecov-io
Copy link

codecov-io commented May 7, 2019

Codecov Report

Merging #367 into develop will increase coverage by 0.29%.
The diff coverage is 83.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #367      +/-   ##
===========================================
+ Coverage    70.73%   71.02%   +0.29%     
===========================================
  Files           59       60       +1     
  Lines         5204     5336     +132     
  Branches       618      659      +41     
===========================================
+ Hits          3681     3790     +109     
- Misses        1431     1449      +18     
- Partials        92       97       +5
Flag Coverage Δ
#javascript 71.02% <83.09%> (+0.29%) ⬆️
#python 78.73% <83.09%> (+0.11%) ⬆️
Impacted Files Coverage Δ
src/smif/data_layer/file/file_data_store.py 87.07% <100%> (ø) ⬆️
src/smif/data_layer/store.py 90.51% <76.82%> (-4.76%) ⬇️
src/smif/data_layer/results.py 91.52% <91.52%> (ø)
src/smif/data_layer/data_array.py 86.89% <0%> (+0.68%) ⬆️

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 a5d2e47...b94bce9. Read the comment docs.

@fcooper8472 fcooper8472 merged commit 7a751da into develop May 7, 2019
@fcooper8472 fcooper8472 deleted the i359_programmatically_query_results branch May 7, 2019 13:27
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.

5 participants