Open fwitte opened 1 month ago
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 "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.
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.
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.
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"])
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)
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.
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.
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.
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
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.
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).
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 :)
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.