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

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

rht
Copy link
Contributor

@rht rht commented Dec 5, 2018

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

@rht
Copy link
Contributor Author

rht commented Dec 8, 2018

@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
Copy link
Contributor Author

rht commented Dec 8, 2018

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.

@rht rht mentioned this pull request Dec 9, 2018
@jsabuco
Copy link
Collaborator

jsabuco commented Dec 9, 2018 via email

@jsabuco
Copy link
Collaborator

jsabuco commented Dec 13, 2018

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
Copy link
Collaborator

x0range commented Dec 13, 2018

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
Copy link
Contributor Author

rht commented Dec 13, 2018

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
Copy link
Contributor Author

rht commented Dec 14, 2018

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

@rht
Copy link
Contributor Author

rht commented Dec 14, 2018

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
Copy link
Contributor Author

rht commented Dec 20, 2018

Rebased.

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