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 example models #56

Merged
merged 28 commits into from
Feb 29, 2024
Merged

Refactor example models #56

merged 28 commits into from
Feb 29, 2024

Conversation

timmens
Copy link
Member

@timmens timmens commented Feb 28, 2024

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:

  1. tests/test_models contains all the models used in the tests.
  2. 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

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.27%. Comparing base (4f97784) to head (48e6070).

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.
📢 Have feedback on the report? Share it here.

@timmens timmens requested a review from hmgaudecker February 28, 2024 11:41
Copy link
Member

@hmgaudecker hmgaudecker left a 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 😇

Copy link
Member

@hmgaudecker hmgaudecker left a 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.

@timmens timmens merged commit 8539c66 into main Feb 29, 2024
6 checks passed
@timmens timmens deleted the cleanup-models branch February 29, 2024 10:28
Copy link
Member

@hmgaudecker hmgaudecker left a 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):
Copy link
Member

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):
Copy link
Member

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 = {
Copy link
Member

@hmgaudecker hmgaudecker Feb 29, 2024

Choose a reason for hiding this comment

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

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.

ENH: Clean up testing and example models
2 participants