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

Add date and time utilities #3247

Open
wants to merge 66 commits into
base: master
Choose a base branch
from

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Dec 2, 2019

@beutlich beutlich added enhancement New feature or enhancement L: Utilities Issue addresses Modelica.Utilities labels Dec 2, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone Dec 2, 2019
@beutlich beutlich self-assigned this Dec 2, 2019
@HansOlsson
Copy link
Contributor

The general idea is good, but:

  • I don't like the abbreviations in the record (and as outputs): min, mon, ms, even if consistent with "struct tm". (As far as I understand the names should be singular: second, minute, hour, year.)
  • I am unsure if we should have leap-year utilities. The reason is that it leads to people doing their own leap-year-calculations and that can lead to annoying bugs. Having a full set of utilities to avoid the need for that might be a better solution (that includes conversions between date-time and number, and handling of date-time-differences).

I tried to look at the Buildings-library. It uses full names in the records, but unfortunately it contains its own leap-year handling (hopefully correct):
https://github.com/lbl-srg/modelica-buildings/blob/master/Buildings/Utilities/Time/CalendarTime.mo

@beutlich
Copy link
Member Author

beutlich commented Dec 3, 2019

I started with an operator record for TimeType to also allow addition, subtraction etc. That meant, we also would need to consider a TimeDeltaType and conversion to absolute time, which is not straight-forward. Therefore, I rejected this first idea and went for the TimeType record as absolute point in time and without numerical conversion. (String conversion is still to do, but without handling of leap-seconds).

I don't like the abbreviations in the record (and as outputs): min, mon, ms, even if consistent with "struct tm". (As far as I understand the names should be singular: second, minute, hour, year.)

OK, will rename.

I am unsure if we should have leap-year utilities.

Hm, @GallLeo what do you think? @mwetter considers leap years only in range [2020, 2030] which might not be sufficient in a general purpose library.

@beutlich beutlich requested a review from GallLeo December 3, 2019 09:33
@sjoelund
Copy link
Member

sjoelund commented Dec 6, 2019

If you want to consider leap years, daylight savings, etc... You need to work on some absolute unit of s/etc relative from 1970... I think that's the only sane way of dealing with any kind of delta and ambiguity... Just convert between tm and time_t if you want to use that (and run the calculation in external C).

@beutlich
Copy link
Member Author

Here is a first (and incomplete) draft for conversion of date/time to string. Not sure if it is good enough ❔ . Any feedback is appreciated.

@dietmarw dietmarw removed their request for review December 11, 2019 07:19
@beutlich beutlich removed this from the MSL4.0.0 milestone Jan 7, 2020
@beutlich
Copy link
Member Author

beutlich commented Apr 1, 2020

@HansOlsson @DagBruck @t-sommer @StephanZiegler Yesterday, I noticed by chance that there are Testing.Utilities.Time.{DateTime, Duration} available in Dymola as operator records. This is exactly the way I wanted to go here for MSL, too. I believe these utilities came up as necessary side-product when meaasuring time in Modelica. Now, that I am aware of this library, I am also biased with the further development. My question is, if you could think of contributing or sharing Modelica code for further development as OSS, such that I am sure, no copyright will be violated?

@m-kessler
Copy link
Collaborator

We would be happy to move the operator records from the Testing library to the MSL.
@beutlich How shall we proceed? My recommendation would be, that I first push a purely MSL-based version of the operator records to this pull request. Then we can review the code and merge it with your work.

@CLAassistant
Copy link

CLAassistant commented Apr 1, 2020

CLA assistant check
All committers have signed the CLA.

@beutlich
Copy link
Member Author

beutlich commented Apr 1, 2020

@m-kessler Thank you for the generous offer. I rebased the commits of this PR on master and resolved the merge conficts. This PR branch is now ready to be used. I added you as contributor (with access role = Triage) such that you can directly push to this branch (hopefully).

I already created a TimeType record. This can be now enhanced by using the two different operator records from the Testing package. It would be good if we can use the exsting names of the TimeType record components (which are different than the Testing package.)

@beutlich beutlich marked this pull request as ready for review April 10, 2020 21:01
@beutlich beutlich changed the title WIP: Add date and time utilities Add date and time utilities Apr 10, 2020
@beutlich beutlich force-pushed the add-time-utilities branch 4 times, most recently from 8cc2d55 to 47d29e8 Compare April 11, 2020 20:48
@beutlich beutlich added the L: C-Sources Issue addresses Modelica/Resources/C-Sources label Apr 11, 2020
m-kessler and others added 29 commits November 11, 2024 20:41
This requires fromSeconds to be the new default.
And it's not possible anymore to create Durations by specifying e.g. hour only.
But unless you write tests or doc, this is not really needed I would say.
Creating a Duration does not work anymore that way, due to "e547700 - Make Duration constructors unambiguous"
... as the one we had did not catch negative durations with multiple days.
... as this is not supported at the moment.
... to fix inheritance, as its not allowed to extend from an operator record in Modelica 3.4
Conversion for MSL v4.0.0 no longer is suitable, hence migrate to next major version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement L: C-Sources Issue addresses Modelica/Resources/C-Sources L: Utilities Issue addresses Modelica.Utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants