Skip to content

Conversation

@willu47
Copy link
Member

@willu47 willu47 commented Mar 19, 2019

  • Adds data_path arguments to the functions which read in assets and links using read_csv. This allows these functions to be imported and re-used from nismod2/models/digital_comms/run.py

  • Commit 6643d1e simplifies read_parameters which @edwardoughton, you should check before accepting. You can git revert 6643d1e to undo this commit if it is not helpful.

  • Closes Factor out system from interventions and adoption modules to allow use in smif wrapper #82

    • We now pass a list of distribution objects into the various methods in interventions.py and adoption.py rather than a system object. This will allow the decision module (which won't have access to the system object) to use these methods.
    • I also demonstrate how to use test classes and a pytest fixture to setup a system for each of the tests. This is a really useful way to organise the tests and makes it easier to find and fix the bugs. You'll note that the tests now pass system._distributions into the methods whose arguments have changed. The tests make making these sorts of changes really easy to check!
    • I have merged in your changes to the test from master to my branch, which makes to above defunct!

@willu47 willu47 requested a review from edwardoughton March 19, 2019 09:56
@coveralls
Copy link

Coverage Status

Coverage increased (+2.6%) to 47.597% when pulling a7c3e5a on integration into 0963c19 on master.

@edwardoughton edwardoughton merged commit 1e51417 into master Mar 23, 2019
Copy link
Collaborator

@edwardoughton edwardoughton left a comment

Choose a reason for hiding this comment

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

Hi Will,

Thanks for these changes which have been useful. The only issue I can see is the ongoing development of interventions.py. I think the key preference you have is to pass only the asset objects into interventions.py, rather than the system, so I'm taking that into consideration in the redevelopment of the model on my branch.

def update_desirability_to_adopt(self, desirability_to_adopt):
self.adoption_desirability = desirability_to_adopt


Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming this is a PEP8 style preference to have a double line space after a class or function?

################################################################


def get_all_distributions_ranked(system, ranking_variable, technology, reverse_value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

On my branch I have updated the model to work in an asset neutral way, so you can just give it either exchanges, cabinets or distribution points. Therefore I'm not going to accept these proposed changes to interventions.py, as I have new updates that I will make to the master in the next few days which meet and hopefully exceed this integration requirement.

params['payback_period'] = param['default_value']
if param['name'] == 'profit_margin':
params['profit_margin'] = param['default_value']
for name, param in parameters.items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@edwardoughton edwardoughton deleted the integration branch March 23, 2019 08:50
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.

Factor out system from interventions and adoption modules to allow use in smif wrapper

4 participants