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

Trying to solve Issue842 multiyear #994

Merged
merged 4 commits into from
Sep 11, 2018

Conversation

AnaConstantin
Copy link
Contributor

For solving #842.
First of all sorry for taking so long to work on this issue. It has been really challenging to get any free time, let alone IBPSA free time.
Anyways I have done a new implementation focusing only on the ConvertTime.mo block
If the simulation time goes over the time span of the weather file, the weather file is repeated only if the time span is a year or a multiple of a year. By time span I mean: end time stamp - beginning time stamp + average increment. To my mind positive and negative time stamps are treated correctly in this manner.
I did add the condition regarding the shift for the solar radiation, I'm not really sure however that it is absolutely necessary. In my implementation it required an additional parameter, an additonal variable and a pair of equations.
I have used the ReaderTMY3.mo file from the master, as the one in the branch was not updated to the changes from December 2017 regarding the absolute file name.
So please update the branch for the issue, and ideally there should be no more merge conflicts.

@mwetter
Copy link
Contributor

mwetter commented Aug 24, 2018

Todo:

  • Add test cases that show that the new code correctly handles the problems that it ought to fix.
  • See if "average increment of weather data" is really needed, which was introduced in this revision. What if data are not equidistant? This new data makes it backward incompatible.

@mwetter mwetter mentioned this pull request Aug 24, 2018
4 tasks
@AnaConstantin
Copy link
Contributor Author

@mwetter thanks for the reply.
I can add an example where all the different cases are tested. What needs to be done after that is for someone to write the script for the CI.
The average increment was actually something you suggested at the beginning and which I then implemented and did not use the first time, while having the exact second thoughts you mentioned here. But actually it is quite useful, because as explained in the part "Start and end data for annual weather data files" from the documentation of ReaderTMY3, while the file provides data for 8760 hours the difference between start and end timestamp is not one year, but one year (end - start) minus one hour (increment). This was my way of covering for this situation and also I wanted to make a more general implementation, since it might be that users have 2 or 3 years of data with a time interval of 10 minutes or half an hour between data points. Yes data points might not be equidistant but the probability is higher that data is missing rather than there is too much data, so we are still okay. We can introduce a warning or add something in the documentation to make users aware of the fact.
I am as always open to other suggestions.

@mwetter
Copy link
Contributor

mwetter commented Aug 27, 2018

@AnaConstantin
Please add the test files. Once we have testing in place, we can see if we can remove the "average increment of weather data", or at a minimum provide good error handling so we can reject models for which the code won't work correctly.

@tbeu
Copy link
Contributor

tbeu commented Aug 30, 2018

I wonder if

  • the periodic extrapolation feature
  • the u_min/u_max parameters for start and end values

of Modelica.Blocks.Tables.CombiTable1Ds can be of any help here. It all comes with new MSL 3.2.3.

@AnaConstantin
Copy link
Contributor Author

@mwetter I did the examples. They are all under IBPSA.BoundaryConditions.WeatherData.Validation.
I did an example where a weather file of one year is repeated for two additonal months and I wanted to do an example where a weather file for two years is repeated for a third year. This last one is a bit of a trick, as the increments are not equidistant (more or less a month), but it still works. I can explain how the users can do it themselves, but I think it would just be more complicated.
I think this whole repeat the year gimmick only makes sense if the increments are equidistant, otherwise I for one at this time have no better idea how to solve it. So my suggestion: good documentation, and maybe a warning if the weather file seems to be close to one year or a multiple of it and a suggestion for the user how it might be handeld to work with repeating the weather file.
I do not see what the problem with the backwards compatibility is, since all this is just a branch, which I suppose will be deleted once the solution is agreed upon and merged into the master?
@tbeu I think the idea was to solve evrything using as little code as possible. At the very beginning I looked in the C-code for the time tables and tryed to find the functions which get the begin and end time stamps from the weather file, but they were not easily separated from the rest of the code, so I just wrote my own.

@AnaConstantin
Copy link
Contributor Author

One more thing, I know the actual error handling is important, but how do you test it using scripts and CI?
One would need a simulation to terminate and analyze the error message. Is this something you want / can do?

@tbeu
Copy link
Contributor

tbeu commented Sep 4, 2018

Still would be interesting how the issue can be solved if the above mentioned features of CombiTable1Ds are utilized. I believe you only need to check if the time span u_max-u_min is an integer multiple of 31536000 and can set both set repeatWeatherFile and CombiTable1Ds.extrapolation accordingly. I guess you can also relax the assumption on equidistant weather data then.

@AnaConstantin
Copy link
Contributor Author

@tbeu thank you for your suggestion. The issue is that for the weather files which span a year for ReaderTMY3 time span u_max-u_min is not equal to a year. There is a detailed explanation on this under "Start and end data for annual weather data files" in the the documentation of ReaderTMY3.

@mwetter mwetter merged commit c0ed879 into ibpsa:issue842_multiyear Sep 11, 2018
@AnaConstantin AnaConstantin deleted the issue842_multiyear branch October 16, 2018 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants