-
Notifications
You must be signed in to change notification settings - Fork 2
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 example models #56
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 97.69% 98.27% +0.57%
==========================================
Files 47 42 -5
Lines 2521 2433 -88
==========================================
- Hits 2463 2391 -72
+ Misses 58 42 -16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much clearer, thanks a lot!
I have a bunch of comments, most of which are unrelated to the changes made here, though 😇
Co-authored-by: Hans-Martin von Gaudecker <[email protected]>
Co-authored-by: Hans-Martin von Gaudecker <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Hans-Martin von Gaudecker <[email protected]>
…cs/lcm into cleanup-models
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just small stuff left, thanks a lot!
If other models enter the tests, we may want to rename deterministic
to base_deterministic
etc., but no need to do that preemptively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be quite some repetetive code setting up the models/params. In test_analytical_solution
, you are now using _get_iskhakov_2017_model
and _get_iskhakov_2017_params
, which seems very nice.
Would it make sense to re-factor that into a module that is used to generate fixtures across multiple test files?
return model_config | ||
|
||
|
||
def _get_iskhakov_2017_params(disutility_of_work): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we make all parameters modifiable and leave the defaults at the values found in the main specification of the paper?
}, | ||
} | ||
|
||
def _get_iskhakov_2017_model(n_periods): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for the params below, but less important here.
@@ -116,12 +116,12 @@ def _model_solution(n_periods): | |||
solve_model, _ = get_lcm_function(model=model) | |||
|
|||
params = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See main comment.
In this PR, I refactor the example models.
Previously, the example models were located in
src/example_models
but were not used in any of the code but only in the tests. One example model did not have a meaningful test but existed only for experimenting on the server.To change this, I created two new folders:
tests/test_models
contains all the models used in the tests.examples
contain example models that can be used elsewhere or as a reference but do not have a corresponding test.I also added comments and cleaned up the example models in the process. Moreover, while deleting the module
get_model.py
, I updated the regression tests.Closes #46