-
Notifications
You must be signed in to change notification settings - Fork 83
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
Compute weekdays #501
Comments
@mwetter I'd like to merge this into IDEAS. Can I expect a review in the coming days? Otherwise I'll just copy over the code manually for now! |
@rubenbaetens if you're interested to verify as well..? |
@Mathadon My issue is that I think setting t=0 for anything other than Jan 1 midnight is dangerous and we should not allow the user to do so. I believe the parameters I also added some other fixme as old implementations are not deleted, and as you can make the model more efficient by avoiding events. Please see the code. Also, the regression tests fail:
|
Regarding the first and second issue. I agree that this is a problem. This is the reason why I originally suggested to put this model in I will have a look at the error. It worked for me but I guess I made some last-minute changes. Edit: |
converted integers to reals in Annex60/Utilities/Time/CalendarTime.mo for #501
I addressed most of the fixme's. Todo:
|
Setting If we have
|
I prefer to keep the implementation since some people may then easily change to code to get the added functionality (i.e. this is what I will do in IDEAS). There is no need to change ReaderTMY3 although it may be convenient that the model contains date/time/hour etc for occupancy schedules etc. |
I think I fixed everything! |
Can you please address the following:
Note that
|
Revised doc. Made some parameters final. Added defaultComponentName
When timRef=Custom the user may used yearRef to define the year when t=0. This may still be useful if the user wants to use an integer to specify the year, rather than working with the enumeration. I adjusted the parameter comment to clarify this. I removed hourRef, minuteRef, secondRef since this was a bit overkill. Final constant is not what I had liked, but ok. Clearly there was a bug where minutes should indeed equal 30. Not sure how I overlooked that.. Thanks! I updated unit test results. You got me confused now about the Real/Integer output. I thought we changed it to Real outputs to have the same output as minutes? I guess your suggestion was then to not round off the hours? I would like to have a proper (i.e. not decimal) hour, so I should revert to the previous implementation then? Having a state event every hour in an optional model does not seem problematic to me. I therefore prefer to have a clear implementation using |
I revised Examples.CalendarTime to remove the state events, as state events were present in Dymola, JModelica and OpenModelica. Interestingly, the computing time did not decrease much, probably because the events are easy to compute for this model. However, as this model likely is to be used with a controller, I think the new implementation is better as the events are exactly triggered at the full hour, rather than having to be approximated by the state event detection algorithm. This is in my view important because if you have two models that sample at the full hour, such as this model and a controller with a sampling rate of say 2 minutes, then you want the events to be simultaneous. This cannot be guaranteed if one of the model uses a state event, rather than a time event. I also tried an implementation using Clock() but this is not supported yet in JModelica, and neither in IDA-ICE as far as I know. However, there are still some issues, as I think the design with using GMT does not work for other time zones. See the comment near the 'fixme'. |
I addressed the fixme's! I think that (implementation-wise) it does not matter whether time=0 corresponds to unix time stamp GMT or local time as long as I renamed |
I propose to add the functionality to compute weekdays. This requires a link to be made between the Modelica variable
time
and our calendar system. Therefore there must be a way for the user to specify what it means if time = 0. For instance time = 0 may mean that it's eitheretc..
Since the concept of time is linked to the input file I propose to add it in
Annex60.BoundaryConditions.WeatherData.ReaderTMY3
. This way it can also be integrated easily into IDEAS.I'll propose an implementation that is disabled by default.
This relates to open-ideas/IDEAS#534
The text was updated successfully, but these errors were encountered: