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

The class inheritance could be more simple #40

Open
fwitte opened this issue Oct 15, 2024 · 1 comment · May be fixed by #41
Open

The class inheritance could be more simple #40

fwitte opened this issue Oct 15, 2024 · 1 comment · May be fixed by #41

Comments

@fwitte
Copy link
Contributor

fwitte commented Oct 15, 2024

The __init__ methods of the various model classes show quite a bit of dublication once the econ_type parameter is integrated. In my opinion, the parameter could be included in the params dictionary. This would already make a lot of things much easier.

On top of that, there are parts of the code (e.g. setup of the self.nw attribute), that is shared for every model and is not affected by any specification. These parts should be included in the HeatPumpBase.__init__ method. Parts, that are individual to other classes should be added in their __init__ method, but only after calling the super().__init__ method. For this purpose it may also be useful. to create an "in-between" level of abstraction, e.g. a TwoWorkingFluidsHeatPump. which has a different __init__ compared to the SingleWorkingFluidHeatPump. These classes do not have any purpose other than the in-between level of abstraction.

@fwitte fwitte linked a pull request Oct 15, 2024 that will close this issue
@jfreissmann
Copy link
Owner

You are right, that there is a lot of duplication (even beyond the __init__ methods) that stems from the historical growth of the model collection. We were planning to add a HeatPumpCascadeBase class at some point, because their __init__ as well as the generate_state_diagram methods and likely more are pretty redundant, too.

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 a pull request may close this issue.

2 participants