Conversation
dfremont
left a comment
There was a problem hiding this comment.
Hi Beyazit, thanks for making this PR! With a few changes I was able to run everything except the final analysis (see comments), and the code looks OK overall. A few bigger things to improve:
- There should be unit tests for the new functionality. They can be basic, not using a neural network or anything (we want the tests to run without optional dependencies such as stable baselines anyway), but should exercise the main code paths and check that the output is reasonable.
- There should be a new page in the documentation describing the compositional analysis features. It can explain the example in more detail (for example, what do
S,X,SXS, etc. actually mean) and also explain how to use the top-level APIsScenarioBaseandCompositionalAnalysisEngine. The docstrings for those are a good starting point (and you can auto-generate documentation from them, as we do indocs/feature_api.rstfor example), but they need to be fleshed out: for example, the analysis methods accept a list of scenario names, but the example only ever calls them with a single string.
If you can help me solve the issue I had when running analyze.py I will try to run the example again. Thanks!
|
I fixed most of the comments. Specifically: (i) fixed the example (it should run smoothly now -- lmk if it doesn't work), (ii) removed redundant code, (iii) eliminated most of the repetitive code, (iv) added unit tests, and (v) improved README and docstrings. There is still more we can improve esp |
No description provided.