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

I351 list results #358

Merged
merged 9 commits into from
Apr 17, 2019
Merged

I351 list results #358

merged 9 commits into from
Apr 17, 2019

Conversation

fcooper8472
Copy link
Contributor

@fcooper8472 fcooper8472 commented Apr 15, 2019

New behaviour

smif list

smif list will now append an asterisk to model run names if full results exist

smif available_results <run_name>

smif available_results <run_name> will detail the results available for a single <run_name>: for instance (with results):

model run: energy_water_cp_cr
  - sos model: energy_water
    - sector model: water_supply
      - output: cost
        - decision 0: 2010, 2015, 2020
      - output: energy_demand
        - decision 0: 2010, 2015, 2020
      - output: water
        - decision 0: 2010, 2015, 2020
      - output: reservoir_level
        - decision 0: 2010, 2015, 2020
    - sector model: energy_demand
      - output: cost
        - decision 0: 2010, 2015, 2020
      - output: water_demand
        - decision 0: 2010, 2015, 2020

And, without results:

model run: energy_water_cp_cr
  - sos model: energy_water
    - sector model: water_supply
      - output: cost
        - no results
      - output: energy_demand
        - no results
      - output: water
        - no results
      - output: reservoir_level
        - no results
    - sector model: energy_demand
      - output: cost
        - no results
      - output: water_demand
        - no results

Omitting the <run_name> will print all information for every model run.

Closes #351 and #352

Copy link
Member

@willu47 willu47 left a comment

Choose a reason for hiding this comment

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

Great, so I've had a play against NISMOD2 with this, and discovered a few things!

  1. smif list is now very slow. Each model run takes about 3 second, less if it doesn't have any results. This may well be our config code, or disk io? Not sure. I think the quick fix is to make the results functionality optional on the smif list command, and if necessary look into optimisation at a later date. Not a priority at present.
  2. Results read out for smif available_results is really good. I don't think I would ever use it for every single model run as it just fills the screen, but the ability to see this for one model run is handy. I think this meets our requirements for now.

Example for digital comms:

(nismod) vagrant@vagrant:/vagrant$ smif available_results digital_comms_test

model run: digital_comms_test
  - sos model: digital_comms_fixed_only
    - sector model: digital_comms_fixed_network
/vagrant/smif/src/smif/data_layer/file/file_metadata_store.py:159: FionaDeprecationWarning: Use fiona.Env() instead.
  with fiona.drivers():
      - output: adoption_cap
        - no results
      - output: distribution_upgrades
        - no results
      - output: distribution_upgrade_costs_fttp
        - no results
      - output: premises_by_distribution
        - no results
      - output: interventions
        - no results
      - output: distribution_upgrades
        - no results
      - output: distribution_upgrade_costs_fttp
        - no results
      - output: distribution_upgrade_costs_fttdp
        - no results
      - output: premises_upgrade_costs_fttp
        - no results
      - output: premises_upgrade_costs_fttdp
        - no results
      - output: premises_by_distribution
        - no results
      - output: premises_adoption_desirability
        - no results
      - output: premises_connection_statistics
        - no results
      - output: premises_passed_with_fttp
        - no results
      - output: percentage_of_premises_passed_with_fttp
        - no results
      - output: premises_connected_with_fttp
        - no results
      - output: percentage_of_premises_connected_with_fttp
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: premises_passed_with_fttdp
        - no results
      - output: percentage_of_premises_passed_with_fttdp
        - no results
      - output: premises_connected_with_fttdp
        - no results
      - output: percentage_of_premises_connected_with_fttdp
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: percentage_of_premises_with_fttc
        - no results
      - output: lad_premises_with_fttp
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: lad_premises_with_fttdp
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: lad_premises_with_fttc
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: lad_premises_with_adsl
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: coverage
        - no results
      - output: percentage_of_premises_connected_with_fttc
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: percentage_of_premises_connected_with_adsl
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: percentage_of_premises_connected_with_docsis3
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: total_cost
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: total_potential_bcr
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: total_potential_benefit
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: rollout_costs
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020
      - output: rollout_bcr
        - decision 1: 2019
        - decision 2: 2019
        - decision 3: 2020

@willu47 willu47 added this to the Results milestone Apr 16, 2019
Now requires smif list <-c/--complete>
@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

Merging #358 into develop will increase coverage by 0.58%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #358      +/-   ##
===========================================
+ Coverage    70.59%   71.18%   +0.58%     
===========================================
  Files           59       59              
  Lines         5183     5292     +109     
  Branches       615      647      +32     
===========================================
+ Hits          3659     3767     +108     
+ Misses        1431     1430       -1     
- Partials        93       95       +2
Flag Coverage Δ
#javascript 71.18% <100%> (+0.58%) ⬆️
#python 78.99% <100%> (+0.5%) ⬆️
Impacted Files Coverage Δ
src/smif/data_layer/store.py 95.23% <100%> (+0.38%) ⬆️
src/smif/data_layer/file/file_data_store.py 87.61% <0%> (+0.53%) ⬆️
src/smif/decision/decision.py 95.15% <0%> (+1.93%) ⬆️

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 b303134...40ce640. Read the comment docs.

@fcooper8472
Copy link
Contributor Author

@willu47 I have made the complete results checking optional: it's now an additional flag smif list <-c/--complete>.

Would be interesting to what is actually slowing up the smif list, but unfortunately I've not been able to get my dev version of smif working with nismod2...

I suspect the slowdown must be in _get_canonical_expected_results rather than _get_canonical_available_results but it only seems to be reading some YAML config - any ideas?

@tomalrussell
Copy link
Member

I'm happy to ignore appveyor/branch failures here where they relate to conda-forge or dependency errors, so long as appveyor/pr and travis get ✔️s.

Copy link
Member

@willu47 willu47 left a comment

Choose a reason for hiding this comment

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

A few optional suggestions for refactoring to help enable #359. Would be good to also get some tests in place against the helper functions.

src/smif/cli/__init__.py Outdated Show resolved Hide resolved
src/smif/cli/__init__.py Outdated Show resolved Hide resolved
src/smif/cli/__init__.py Outdated Show resolved Hide resolved
@fcooper8472
Copy link
Contributor Author

The new functionality builds the list of available results based on calls to store.canonical_expected_results and store.available_results. This removes some code duplication and should also improve performance.

All methods (including new cli functionality) are now unit tested.

Ready for another review @willu47

@fcooper8472 fcooper8472 requested a review from willu47 April 16, 2019 13:30
@fcooper8472 fcooper8472 mentioned this pull request Apr 16, 2019
Copy link
Member

@willu47 willu47 left a comment

Choose a reason for hiding this comment

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

That all looks great Fergus. I've tested it against the nismod2 repo and it works fine. Thanks a lot!

@fcooper8472 fcooper8472 merged commit 75c3452 into develop Apr 17, 2019
@fcooper8472 fcooper8472 deleted the i351_list_results branch April 17, 2019 09:33
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.

3 participants