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

Automatically collect and test models #39

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

fwitte
Copy link
Contributor

@fwitte fwitte commented Oct 15, 2024

This PR aims to simplify testing by automatically collecting the model classes available to the dashboard.

The tests are carried out by creating an instance of the model class, providing the default parameters from the model parameter .json files and then checking whether the last simulation has converged.

The model registry can also be used to construct the classes in the dashboard itself, potentially also for injecting classes from external locations into a running version of the dashboard.

@fwitte
Copy link
Contributor Author

fwitte commented Oct 15, 2024

I included all of the information in the input json files now, i.e.

  • Fixed the model type references (some were broken)
  • Integrated what was former in the variables.py -> hp_models dictionary into the json files
  • Made the parameters.py (apparently) obsolete. I think we should discuss, how to access the models in this context (see README).
  • Made most tests run again, some models seem to be missing configuration parameters in the input json files

@jfreissmann
Copy link
Owner

The reason for not including the dashboard parameters into the params.json was simply to separate the model input data, that can and should be used outside of the dashboard, from the stuff that is needed in the latter. I agree with you, that the way it is handled now is not perfect either and should be improved.

@jfreissmann
Copy link
Owner

Thank you for finding the erroneous "type" references. Some were definitely wrong, but a great chunk were including the economizer type, which was a design decision, as the "type" field originally was not ment to be a direct reference to its class, but rather a fully defined topology that could be used in e.g. filenames.

@jfreissmann
Copy link
Owner

I really like the idea of the model_registry and the way you were able to reduce the test code. I think, we will have to look into how users, who only want to use the models without the dashboard, can access the former easily and intuitively, which in my opionion felt better the way it was before using get_params and importing the model class. Maybe there is a good compromise between those.

@fwitte
Copy link
Contributor Author

fwitte commented Oct 16, 2024

think, we will have to look into how users, who only want to use the models without the dashboard, can access the former easily and intuitively, which in my opionion felt better the way it was before using get_params and importing the model class. Maybe there is a good compromise between those.

This still works in principle :). I renamed the heat pump models to include the open or closed to ...Open and ...Closed (it could be anything really). This does also eliminate the necessity to pass the econ type to the get_param method. It is encoded in the name of the model, which again knows its parameter settings including the econ_type from the input .json files.

Actually, the HeatPumpEconIHX could be also changed to handle the econ_type inside the params dictionary so there is no need for an extra keyword. And, finally the user also does not have to actually import the class. With the model_registry, we can also create a method, that just returns the instance for a given parameter setting.

@fwitte
Copy link
Contributor Author

fwitte commented Oct 16, 2024

params = get_params('HeatPumpEconIHXClosed')
params['ihx']['dT_sh'] = 7.5  # superheating by internal heat exchanger
hp = HeatPumpEconIHX(params=params, econ_type=params["setup"]["econ_type"])

@fwitte
Copy link
Contributor Author

fwitte commented Oct 16, 2024

And, finally the user also does not have to actually import the class. With the model_registry, we can also create a method, that just returns the instance for a given parameter setting.

from heatpumps.parameters import get_params, get_model

params = get_params('HeatPumpEconIHXClosed')
params['ihx']['dT_sh'] = 7.5  # superheating by internal heat exchanger
hp = get_model(params)
print(hp)

@fwitte
Copy link
Contributor Author

fwitte commented Oct 17, 2024

One more side note: right now all json files are loaded on startup, this takes a bit of time. I suggest, that the data can be loaded on demand and added to the hp_models dictionary when required. This would minimize the i/o operations.

@jfreissmann
Copy link
Owner

from heatpumps.parameters import get_params, get_model

params = get_params('HeatPumpEconIHXClosed')
params['ihx']['dT_sh'] = 7.5  # superheating by internal heat exchanger
hp = get_model(params)
print(hp)

I think, this looks good to me. Maybe the method should be called get_model_by_params or something like that.

@jfreissmann
Copy link
Owner

One more side note: right now all json files are loaded on startup, this takes a bit of time. I suggest, that the data can be loaded on demand and added to the hp_models dictionary when required. This would minimize the i/o operations.

The idea with that decision was, that for the enduser the initial start up time is less important than the responsiveness when switching between models. For the programmatic interface I agree, that only necessary parameters should be loaded in.

@jfreissmann
Copy link
Owner

On another note:

As you most likely inserted the dashboard info programmatically into the input parameters, there was an issue with UTF-8 encoding, so umlauts are not correctly inserted. See for example this file:

src/heatpumps/models/input/params_hp_cascade_econ_closed_ihx.json

Excerpt
"refrig2": "R717",
"base_topology": "Kaskadierter Kreis",
"display_name": "Geschlossen | Reihenschaltung | interne W\u00dcT (Variante B)",
"nr_ihx": 2,
"econ_type": "closed",
"comp_var": "series",
"nr_refrigs": 2,
"process_type": "subcritical"

Furthermore, having the dashboard text inside the file could be problematic with the multi language support.

@fwitte
Copy link
Contributor Author

fwitte commented Oct 18, 2024

That thing could easily be reverted. It was just an idea to have things in one place.

@fwitte
Copy link
Contributor Author

fwitte commented Oct 18, 2024

One more side note: right now all json files are loaded on startup, this takes a bit of time. I suggest, that the data can be loaded on demand and added to the hp_models dictionary when required. This would minimize the i/o operations.

The idea with that decision was, that for the enduser the initial start up time is less important than the responsiveness when switching between models. For the programmatic interface I agree, that only necessary parameters should be loaded in.

Actually, the respective params json was loaded on demand every time when selecting the working fluid earlier. Now everything is loaded on startup, that is why it takes a little bit more. I suggest a function that is tied to the dashboard startup to load all the params, and one to load on demand (e.g. when importing a model separately as the example in the docs show).

@fwitte
Copy link
Contributor Author

fwitte commented Oct 18, 2024

from heatpumps.parameters import get_params, get_model

params = get_params('HeatPumpEconIHXClosed')
params['ihx']['dT_sh'] = 7.5  # superheating by internal heat exchanger
hp = get_model(params)
print(hp)

I think, this looks good to me. Maybe the method should be called get_model_by_params or something like that.

Sure! You can also go ahead and push on my branch :)

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.

2 participants