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

Decision module may access available interventions #339

Merged
merged 12 commits into from
Apr 11, 2019

Conversation

willu47
Copy link
Member

@willu47 willu47 commented Mar 29, 2019

Added two layers of set operations in the DecisionManager and DecisionModule and addresses issue #333. DecisionManager now updates pre-decision state (from the previous iteration) with new decisions and writes this to the store for current timestep and iteration.

This is a requirement of nismod/nismod2#82

Consider set of all interventions
Set of planned interventions
Available interventions
Decisions at time t

Todo:

  • Update pre-specified and decision module decisions using technical_lifetime attribute
  • Check that pre-specified planning and decision module approaches work together

This PR now also removes the pre-specified planning decision module, instead treating initial conditions and pre-specified planning as state which is all dealt with in the get_and_save_decisions method in the decision manager.

@willu47 willu47 changed the base branch from master to develop April 1, 2019 11:10
Added two layers of set operations in the DecisionManager and DecisionModule

Consider set of all interventions $I$
Set of planned interventions $P \subset I$
Available interventions $A = P \cap I$
Decisions at time t $D_t \subset A - D_{t-1}$
@willu47 willu47 marked this pull request as ready for review April 4, 2019 10:13
@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #339 into develop will increase coverage by 0.13%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #339      +/-   ##
===========================================
+ Coverage    70.43%   70.57%   +0.13%     
===========================================
  Files           59       59              
  Lines         5135     5179      +44     
  Branches       608      615       +7     
===========================================
+ Hits          3617     3655      +38     
- Misses        1426     1431       +5     
- Partials        92       93       +1
Flag Coverage Δ
#javascript 70.57% <90%> (+0.13%) ⬆️
#python 78.47% <90%> (+0.08%) ⬆️
Impacted Files Coverage Δ
src/smif/data_layer/abstract_data_store.py 100% <100%> (ø) ⬆️
src/smif/data_layer/store.py 94.84% <100%> (ø) ⬆️
src/smif/decision/decision.py 93.27% <89.68%> (-0.06%) ⬇️
src/smif/data_layer/data_handle.py 82.8% <0%> (-1.21%) ⬇️

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 841615b...1f0d0f2. Read the comment docs.

@willu47 willu47 requested a review from tomalrussell April 4, 2019 16:10
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.

Looks good 👍 - neat simplification of pre-specified planning, clear processing for the planned/decided/retired sets.

I've suggested a couple of minor changes in comments.

Could we use some of the PR comment/docstrings content to start a doc page (after 'Adding a Model') while this is fresh?

  • Consider set of all interventions
  • Set of planned interventions
  • Available interventions
  • Decisions at time t

#347 and #348 look like useful follow-ups

src/smif/decision/decision.py Outdated Show resolved Hide resolved
src/smif/decision/decision.py Outdated Show resolved Hide resolved
src/smif/decision/decision.py Outdated Show resolved Hide resolved
@tomalrussell tomalrussell merged commit 595d53d into nismod:develop Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants