search

ibpsa / modelica-ibpsa

Modelica library for building and district energy systems developed within IBPSA Project 1
https://ibpsa.github.io/project1
142 stars 84 forks source link

Update Weather diffuse radiation model #1477

Closed EttoreZ closed 2 years ago

EttoreZ commented 3 years ago

From the weather BESTEST analysis emerged that that the IBPSA library gives slightly different results with respect to other software tools (TRNSYS and E+) for diffuse radiation on a tilted surface. By analyzing the results, documentation and source code I found that the main differences are:

mwetter commented 3 years ago

@EttoreZ : Please use the branch issue1477_diffuseRadiation and issue1478_BESTEST_weatherfiles which I just created

Mathadon commented 3 years ago

@EttoreZ nice work!

EttoreZ commented 3 years ago

An update after a long silence. I was planning on merging the changes today but I reached an impasse that requires some attention. Basically among the fixes and improvements I made to the Perez diffuse radiation model, there is one on the calculation of the relative air mass that takes into account the altitude of the location, which in theory is present on the TMY3 file and I implemented a way to read it. However the latitude, which could also be read directly from the TMY3 is not used in the diffuse radiation model, it is instead left as a parameter that the user is inserting themselves. Given the situation I have the following suggestions: • The parameter “alt” for altitude becomes something the user has to insert themselves (this though will destabilize all the library models that use the Perez Diffuse radiation model unless a default parameter is chosen) • Make the latitude and altitude switchable between a real input coming from the weather file and a user defined parameter. In this way the implementation will be harmless to all the pre-existing models and also add a new feature Let me know what you think @mwetter @Mathadon

Mathadon commented 3 years ago

@EttoreZ good to hear from you again! You seem to be using both 'altitude' and 'latitude' in the explanation above. I presume that you only mean altitude (in meters, there is also the solar altitude angle)?

I would propose an implantation similar to what we currently have for lat. Read it from TMY3 such that it is available:

  final parameter Modelica.SIunits.Angle lat(displayUnit="deg")=
    IDEAS.BoundaryConditions.WeatherData.BaseClasses.getLatitudeTMY3(
    filNam) "Latitude";

Library developers can use that variable if they want, or allow overrides by the end user if they want. I would not make it a RealInput since those are not parameters, while this variable is a parameter.

EttoreZ commented 3 years ago

@Mathadon I am sorry for the ambiguity. I will try to be more clear. When I referred to the latitude I meant exactly the parameter you quoted "lat" as an example on how to deal with the new parameter "alt" as in you pointed out the altitude of the location. in the TMY3 reader the parameter alt is available as such:

  final parameter Modelica.SIunits.Length alt(displayUnit="m")=
    IBPSA.BoundaryConditions.WeatherData.BaseClasses.getAltitudeLocationTMY3(filNam)
    "Altitude";

Now the question arises in this model IBPSA.BoundaryConditions.SolarIrradiation.DiffusePerez, where both the lat and now also the alt parameter are needed. The "lat" parameter is currently let to the user to decide and not read from the weather file. I can do the same with the "alt" as you mentioned like this:

  parameter Modelica.SIunits.Angle lat "Latitude";
  parameter Modelica.SIunits.Angle alt "Altitude";

By doing this though I guess people need to be aware of the fact that their models using the DiffusePerez model with the current implementation will not run after updating the library because of a missing parameter. Regarding the real input implementation, I know it's not the best to convert a parameter into a real input, but that is how the weaDat/weaBus handles those parameters (alt,lat,lon). I don't know of way of keeping it a parameter and propagate it to other models unless we use the function to re-read it from the weather file. If you know a way to leave as default the parameter read from the weather file and then give the possibility to overwrite it with a boolean switch, that could be the less problematic solution I guess. Another way could be to put as default an "alt" value that give the exact same result of the previous model without the alt parameter. 

  parameter Modelica.SIunits.Angle alt = 0 "Altitude";

I hope I was more clear in my explanation.

Mathadon commented 3 years ago

Ok that clears things up a bit!

So the concern is mostly about backwards compatibility? Simply set default alt=0 for that like you suggest.

In IDEAS we have

  parameter Modelica.SIunits.Angle lat(displayUnit="deg") = weaDat.lat
    "Latitude of the location"
    annotation(Dialog(tab="Advanced"));

in our IDEAS.BoundaryConditions.Interfaces.PartialSimInfoManager where weaDat is IDEAS.BoundaryConditions.WeatherData.ReaderTMY3. This solution would also work for the new parameter and the variable could stay a parameter that way.

EttoreZ commented 3 years ago

I see, then I will implement it putting alt = 0 as standard value. Using weaDat.alt I am afraid some users might have changed the weaDat default name for whatever reason and than it would not work or create inconsistencies in the case of multiple weaDat istances. I will make the pull request this week. Thank you for your help @Mathadon!

Mathadon commented 3 years ago

Of course, you should only make reference to a component that has been declared in the same model, which is the case in IDEAS.

mwetter commented 3 years ago

I think best would be to add altitude to the weather data bus, get its values from the weaBus connector in DiffusePerez (this bus connector already exists), and set the value in ReaderTMY3. Then, we have both, lat and alt on the weather bus (lat is already on the weather bus).

Then the parameter lat in DiffusePerez can be removed and a conversion script can be provided. For some reason, DiffusePerez is currently requesting the value for lat as a parameter. (And for what it is worth, IBPSA.BoundaryConditions.SolarIrradiation.Examples.DiffusePerez sets the parameter to a wrong value, which is different than the value from the weather file.)

This way, we can obtain lat and alt from the weather data bus, minimizing the number of parameters the user has to enter and mistakes the user may do (as in the above example).

For the record, DiffusePerez is used in Buildings in the room model, the thermal solar collector and in the PV examples. For all of them, the user needs to manually enter the latitude (and with the above change also the altitude). All of these models connect to the weather bus, so getting these from the weather bus would be my preferred option and the overhead is likely small (a tool could optimize it, or if not, the cost to compute them is small as they don't enter nonlinear equations).

EttoreZ commented 3 years ago

I am almost ready to make the pull request according to Michael specifications. The conversion script is still missing. However, If we want to compromise between the approach proposed by @mwetter and suitable for the Buildings and the approach proposed by @Mathadon and suitable for IDEAS I guess introducing the switch input/parameter would allow a smoother implementation even without the conversion script, what do you think?

mwetter commented 3 years ago

@EttoreZ : Once alt is on the weather bus, it will also be accessible for IDEAS. So I don't see a compelling reason for a switch. Do you agree @Mathadon ?

Mathadon commented 3 years ago

I agree with @mwetter

EttoreZ commented 3 years ago

Ok perfect, I just need to implement the conversion script then and make the pull request. Is there a guideline to follow regarding how to implement and where to place the conversion script in the library ?

mwetter commented 3 years ago

@EttoreZ : Please add the conversion commands to the top of Resources/Scripts/Dymola/ConvertIBPSA_from_3.0_to_4.0.mos

EttoreZ commented 3 years ago

@mwetter while updating the lat parameter I noticed that it propagates from the calculations of IBPSA.BoundaryConditions.SolarGeometry.ZenithAngle and IBPSA.BoundaryConditions.SolarGeometry.IncidenceAngle which are also used in IBPSA.BoundaryConditions.SolarIrradiation.DirectTiltedSurface and IBPSA.BoundaryConditions.WeatherData.ReaderTMY3 components. Can I convert all the lat parameters into inputs in this issue (and consequent pull request) ? I am asking because if I only change the angles and the PerezDiffuse the inclined radiation model and ReadTMY3 would not work. Furthermore for the conversion script, I should only add the conversion for IBPSA.BoundaryConditions.SolarIrradiation.DiffusePerez and IBPSA.BoundaryConditions.SolarIrradiation.DirectTiltedSurface by adding the two commands : convertElement( "IBPSA.BoundaryConditions.SolarIrradiation.DiffusePerez", "incAng.incAng.decAng1", "incAng.incAng.lat"), convertElement( "IBPSA.BoundaryConditions.SolarIrradiation.DirectTiltedSurface", "incAng.incAng.decAng1", "incAng.incAng.lat")

All the other updates to the library:

should not get a conversion script, correct?

Lastly, once everything is ready, the pull request should be tagged as not backward compatible?

mwetter commented 3 years ago

@EttoreZ : The parameter lat should be removed in all class definitions, and instead the value should be used from the weather data bus (where the model has already a weather data bus) or a RealInput should be added if it is in BaseClasses and the block has not yet a weather data bus.

I don't understand the convertElement examples you have in your comment. This command cannot be used to change from a parameter to a RealInput. If the class is in BaseClasses, then just change it. If it is not in BaseClasses, then

  1. if it is just removing a parameter, then use something like 
    convertModifiers("IBPSA.Airflow.Multizone.Orifice", {"m_flow_small"}, fill("",0), true);
  2. if it requires adding a new input connector, then add something like 
    convertClass("IBPSA.Fluid.Sources.FixedBoundary",
         "IBPSA.Obsolete.Fluid.Sources.FixedBoundary");

    make a copy in the Obsolete folder, add the obsolete warning (see the other models in this package) and add the input connector to the model that is not in Obsolete. This way, if a user uses the class, the user's model will be converted to use the version from Obsolete and the user's model will still work, and it will issue a warning about the use of an obsolete model. In both cases, make also an entry in the revision notes.

The examples such as SimpleRoomTwoElements and the validations such as IBPSA.BoundaryConditions.Validation.BESTEST.WD100 are not included in the conversion scripts. Simply remove the parameter assignment for lat in the affected instance, and make an entry in the revision notes.

Yes, the pull request should be tagged as non-backward compatible.

EttoreZ commented 3 years ago

@mwetter just made a pull request https://github.com/ibpsa/modelica-ibpsa/pull/1517, regarding the conversion script in the end it was only required to remove the parameter to non baseclasses models.