Issue1125 st ro be

[Mathadon]

closes #1125

[Mathadon]

@cprotopa do you agree with these changes?

[cprotopa]

Concerning mDHW I think a larger update is needed.
StROBe actually outputs liters/minute water withdrawal. Here there is a division by 60 to make it in l/s. We need to see what would be the best units here. Would SI not be m3/s? Also, it's not really "mass flow". A big discussion, I suggest to open in another issue.

[Mathadon]

SI is kg/s. Best to discuss this in a separate issue indeed, if it's really an issue :)

[cprotopa]

The model works fine for me, so I believe the changes are correctly implemented.
Of course, the change from Celsius to Kelvin makes sense. We should just try to inform as many people as possible, because they might have adapted their code elsewhere to perform this change, and this will cause a problem.

Since we are at it, though, I would suggest we make some more thorough clean up of IDEAS.BoundaryConditions.Occupants. Specifically I think everything in the following folders is totally outdated and not used anywhere, so it could even be removed.

• IDEAS.BoundaryConditions.Occupants.Extern.Files
• IDEAS.BoundaryConditions.Occupants.Extern.Interfaces
• IDEAS.BoundaryConditions.Occupants.StROBe
• IDEAS.BoundaryConditions.Occupants.Examples.Occupant

Btw, also:

• IDEAS.BoundaryConditions.Climate.Time.Elements

Perhaps this should be done in a separate pull request? What would be best, @Mathadon ?

[Mathadon]

Yup, good to make a separate issue/pull request for that!

The C-to-K conversion I did not notice before and is indeed a bit dangerous. @jelgerjansen was this intentional, and what is the rationale for making the change?

[jelgerjansen]

@Mathadon I made the change because the other temperatures (eg. the indoor building temperature) are expressed in Kelvin and it made more sense to me to do the conversion at the lowest level.
But the manipulation from C-to-K can of course also be done in my own models.
(We could always adapt the documentation such that it's clear for the users that the setpoints are expressed in degrees Celcius)

[Mathadon]

That’s true, but as Christina points out, there are many people that are accustomed now to having this variable in C. @cprotopa , I propose to make the conversion but perhaps change the variable name such that users get an error because the older variable no longer exists?

[cprotopa]

Hm.. Which variable though? If it's locally changed, say we change the name of TSet_signal, nobody will notice, as it's not used elsewhere.
If, on the other hand, we change the RealOutput TSet of the IDEAS.Templates.Interfaces.BaseClasses.Occupant to TSetK, then it should be also changed in all occupant models in the library that use the template, and in the connection equation in IDEAS.Templates.Interfaces.Building. In order for it to be noticed by users, then it should be also changed in IDEAS.Templates.Interfaces.BaseClasses.HeatingSystem, and all models that use this latter one.
To me, this seems like the only way to propagate the change of a variable name in all components of the library, making sure that somebody who made a custom component will get an error message.
Or am I missing something..?
Of course, they will also probably get some warning of too high temperature here and there. Is this perhaps an approach? Set some max value for the input, that would create a warning? At least we could cover the case where somebody altered the input file to contain K already. Just a thought...

[Mathadon]

Uhm, yes that would definitely be an option. I wasn't sure where the input actually comes from. It's set by a user? Or is it read from file? In both cases we could indeed add a warning/error..

[cprotopa]

StrobeInfoManager reads external text files, which the user chooses. Normally, these are the ones generated by StROBe, so they are in degrees C. But I can imagine somebody could have chosen to post-process these files to be in K.

In fact, TSet of the Occupant template is actually specified as

 Modelica.Blocks.Interfaces.RealOutput[nZones] TSet(
    final quantity="ThermodynamicTemperature",
    unit="K",
    displayUnit="degC",
    min=0) 

It is also used as such for example in IDEAS.Templates.Heating.BaseClasses.HysteresisHeating, where TSensor is directly subtracted from TSet. Makes sense to have all temperatures everywhere in SI, of course.

Another option I can think of, is to actually change the StROBe python code to produce the files in K in the first place. This means we would leave the model in IDEAS as is, and assume input files are appropriately in K.
We can perhaps additionally use a boolean in the StROBe component that would specify whether the input is in C or K, and change it or not. Perhaps here can be a check (based on temperature values?).

[Mathadon]

The correct approach indeed seems to be to make the change on the strobe side then and to leave the current implementation as it is. @jelgerjansen can you revert the temperature conversion? @cprotopa I’ll leave it up to you to decide whether or not you’d like to ‘fix’ strobe? Thx!

[cprotopa]

Ok, then I will change StROBe.

What can be added for clarity in IDEAS.BoundaryConditions.Occupants.Extern.StrobeInfoManager is to include the expected units for the inputs. For example this can be at least added in the string parameters comments. E.g.:

 parameter String FilNam_mDHW = "none.txt"   "File with hot water tapping profiles (in l/min)" ;
 parameter String FilNam_QCon = "none.txt"    "File with (convective) internal heat gain profiles (in W)" ;
 parameter String FilNam_TSet = "none.txt"    "File with (main) space heating setpoint profiles (in K)";

The units are W for P,Q, Q_Con, Q_Rad, PHp, and PPv. It's then K for TSet and TSet2, and l/min for mDHW (this last one we will discuss in another issue).

@jelgerjansen could you also add this? Otherwise I will do it afterwards.

[jelgerjansen]

I reverted the temperature conversion and updated the documentation. (@Mathadon will add my last commit to this issue)

[Mathadon]

this is ready to merge when unit tests pass