search

INET-Complexity / isle

11 stars 20 forks source link

[REFACTOR] Move metainsuranceorg.py to a folder agents/ #108

Open rht opened 5 years ago

rht commented 5 years ago

Untested yet (pending Travis CI). Once tested I will move the rest of the agents to agents/ as well.

rht commented 5 years ago

@x0range @jsabuco I tested locally start.py works fine. I have since moved all the agent definition files to agents/, all contracts to contracts/, which unclutters the root dir.

rht commented 5 years ago

Furthermore, maybe compute_profits_losses_from_cash.py, all the obsolete plotting code, visualization should be moved to a folder called analysis/. Without the folder organization, I have to deduce whether a file is an agent specification, a contract, a data analysis script, etc from scratch.

jsabuco commented 5 years ago

I am not opposed to create folders to organize the code.

Which is your view on this Torsten?

On Wed, 5 Dec 2018 at 01:53, rht notifications@github.com wrote:

Untested yet (pending Travis CI). Once tested I will move the rest of the agents to agents/ as well.

You can view, comment on, or merge this pull request online at:

https://github.com/INET-Complexity/isle/pull/108 Commit Summary

  • [REFACTOR] Move metainsuranceorg.py to a folder agents/

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/INET-Complexity/isle/pull/108, or mute the thread https://github.com/notifications/unsubscribe-auth/AKBvlEdQ5_IH5chrnRxqwENX5M45QNtgks5u1ycSgaJpZM4ZB2nQ .

jsabuco commented 5 years ago

Another concern here is that for the moment we cannot run models with subdirectories in the cloud that we are using. It will probably change soon. I will wait until then before merging this pull request.

Thank you very much for the suggestion!

x0range commented 5 years ago

Three comments on this:

  1. Submodules should be organized to comprise relatively separate segments of the code base, ideally classes that rely mostly only on other components of the submodule. While for other ABM projects it may make sense to have an agents submodule (comprising behavioral algorithms etc), I worry that in our case, agents are not sufficiently separate from the rest of the project (observe how the agent objects import stuff from all over the place).
  2. I agree that the flat directory structure is getting messy. Perhaps it is possible to move plotting and visualization stuff to a submodule. Or possibly the riskmodel stuff. (Not sure that makes the structure clearer.)
  3. Most importantly in the short term: sandman2 does in its current version (or perhaps just in the way it is deployed at sandtable) not work with submodules - we tested it yesterday. Therefore, I am strongly in favor of delaying this until that is taken care of at Sandtable.

Edit: I did not see that Juan had also just mentioned the sandman2 error connected to submodules.

rht commented 5 years ago

OK, I see. What are the import errors caused by the submodules? They could be solved (not sure by which one) with either relative import, __init__, or appending to sys.path: sys.path.append(os.path.dirname(__file__)).

While for other ABM projects it may make sense to have an agents submodule (comprising behavioral algorithms etc), I worry that in our case, agents are not sufficiently separate from the rest of the project (observe how the agent objects import stuff from all over the place).

The agents do import from contracts and riskmodel, even so, the import structure is not circular, and it should be fine to put the agents into one folder. The contracts are self-contained, on the other hand. The riskmodel, distributionreinsurance group are also self-contained.

I will investigate the quirks of sandman2 in the meantime.

rht commented 5 years ago

(I rebased to remove the merge conflict in the meantime)

rht commented 5 years ago

I added __init__.py to contracts, and tested that start.py runs regardless of the current directory of the shell. I didn't expose metainsurancecontract in the __init__.py though.

rht commented 5 years ago

Rebased.