-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: main
Are you sure you want to change the base?
Automatically collect and test models #39
Conversation
…in the parameter files
I included all of the information in the input json files now, i.e.
|
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. |
Thank you for finding the erroneous |
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 |
This still works in principle :). I renamed the heat pump models to include the Actually, the |
…le to load the respective data
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"]) |
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) |
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 |
I think, this looks good to me. Maybe the method should be called |
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. |
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. |
That thing could easily be reverted. It was just an idea to have things in one place. |
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). |
Sure! You can also go ahead and push on my branch :) |
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.