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

DRY optimization core layer #96

Open
wants to merge 9 commits into
base: remove/pointless-tabulate-parameter
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jul 1, 2024

For #94, @danielhuppmann asked in the core layer: "Why does Parameter not inherit from Table"? And sure enough, these optimization items share a lot of common functionality. So before moving on to Variable and Equation, for which this will be the case even more so, I wanted to take another look at this.

@meksor, I expect you'll be the one to take a look at this. Sorry, this description might be a bit lengthy.

Initially, I interpreted the question as for the DB layer, and how to handle inheritance (or rather Composition) there has been recently discussed in #82. Instead, this PR focuses on the core layer, where we also have two classes per optimization item that could potentially benefit from higher reusability (to follow the DRY principle): e.g., the Table (mostly a collection of data attributes) and the TableRespository (functions to handle Tables).
For the Tables themselves (and similar), we are currently using Python's property to make data attributes accessible in a controlled way. Unfortunately, these propertys don't mix well with inheritance. This article describes how to still make it work for the getter part, but since we would still have to repeat the same number of lines in the data files, I'm not sure it's worth the effort here. What we stand to gain is a separation of functionality and placeholder code, it seems to me. So when we want to make changes to how e.g. data is returned from an optimization model, we would just need to change one place and all others would inherit these changes. However, code repetition would not be decreased and understanding the models might become more complicated, so I've put this on hold for now.
For TableRepository, there is luckily a quite straightforward way: we can employ the same approach as in the DB layer and create an OptimizationBaseRepository in the core layer that can be extended with Creator, Retriever, etc as needed. This seems to work well and every optimization item repo then just needs to clarify which _model_type and _backend_repository it is interested in.
For better understanding, I wanted to type hint the _backend_repository in the OptimizationBaseRepository. However, we didn't already have an appropriate object to do so. That's why I have created a BackendBaseRepository in abstract.optimization.base, which simply announces how the actual TableRepository in the DB layer is going to look. I'm not sure if this is the correct location; I've usually worked with abstract models to type hint API/DB layer functionality, but I also don't know where else it would live.

This is the point where I'm not sure I'm naming things correctly anymore. I started from Table, which already has Table.data, which is arguably the largest part of shared functionality, at least in the DB layer, and which is missing from IndexSet and Scalar. So these two items could not use the OptimizationBaseRepository as it is now, which seems like a misnomer. Maybe the data functionality needs to be further separated (allowing us to DRY even more parts of this layer) or maybe we find another name?
The same applies for BackendBaseRepository.


Note

For the curious: there's an issue on CPython about propertys and inheritance from 2012.

@glatterf42 glatterf42 added the enhancement New feature or request label Jul 1, 2024
@glatterf42 glatterf42 self-assigned this Jul 1, 2024
@meksor
Copy link
Contributor

meksor commented Sep 17, 2024

@glatterf42 OK so i think the currently proposed changes (removing the commented base model) are good. I also think the rest of the facade layer could also benefit from something like this. Lets figure the @property part out later, for now just explicitly having them in every leaf class is okay imo.
Since we are now not depending on sqlalchemys session-synchronization anyway (changing something in a model with id=1 will change it in all models referencing that row) it would also be okay to just reassign the properties (maybe automatically) to the facade class from the api or db model, thereby completely removing the whole property thing for most stuff.

Copy link
Contributor

@meksor meksor left a comment

Choose a reason for hiding this comment

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

noticed that i never approved this

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 99.00990% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.9%. Comparing base (50d6fae) to head (74e773f).

Files with missing lines Patch % Lines
ixmp4/core/optimization/base.py 97.0% 1 Missing ⚠️
Additional details and impacted files
@@                          Coverage Diff                          @@
##           remove/pointless-tabulate-parameter     #96     +/-   ##
=====================================================================
- Coverage                                 86.9%   86.9%   -0.1%     
=====================================================================
  Files                                      228     230      +2     
  Lines                                     8135    8142      +7     
=====================================================================
+ Hits                                      7075    7081      +6     
- Misses                                    1060    1061      +1     
Files with missing lines Coverage Δ
ixmp4/core/optimization/equation.py 93.5% <100.0%> (-0.7%) ⬇️
ixmp4/core/optimization/indexset.py 91.5% <100.0%> (-1.2%) ⬇️
ixmp4/core/optimization/parameter.py 93.3% <100.0%> (-0.8%) ⬇️
ixmp4/core/optimization/scalar.py 93.8% <100.0%> (-0.7%) ⬇️
ixmp4/core/optimization/table.py 92.5% <100.0%> (-0.9%) ⬇️
ixmp4/core/optimization/variable.py 93.6% <100.0%> (-0.6%) ⬇️
ixmp4/data/abstract/__init__.py 100.0% <ø> (ø)
ixmp4/data/abstract/optimization/__init__.py 100.0% <100.0%> (ø)
ixmp4/data/abstract/optimization/base.py 100.0% <100.0%> (ø)
ixmp4/data/abstract/optimization/equation.py 80.0% <100.0%> (+0.5%) ⬆️
... and 6 more

@glatterf42
Copy link
Member Author

Please note: with d964068, this PR is fine to be merged to main. All tests are passing and it was approved.

However, it adds a lot of untyped code, so I will rebase it onto #129's branch and resolve all merge conflicts/add type hints here, too. In fact, I'll even use #138's branch since that makes even more relevant changes. Once that works, we can merge it; should it turn out to be too complicated, we can still revert to the commit mentioned above.

@glatterf42 glatterf42 changed the base branch from main to remove/pointless-tabulate-parameter November 27, 2024 14:25
@glatterf42 glatterf42 force-pushed the remove/pointless-tabulate-parameter branch from 50d6fae to 79b2c14 Compare November 29, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants