Skip to content

Support time points with unit greater equal days#3821

Merged
MridulS merged 4 commits intomainfrom
support-large-time-points
Jan 27, 2026
Merged

Support time points with unit greater equal days#3821
MridulS merged 4 commits intomainfrom
support-large-time-points

Conversation

@jl-wynen
Copy link
Copy Markdown
Member

@jl-wynen jl-wynen commented Jan 21, 2026

This is now possible because we use C++23.

Fixes #3725

@jl-wynen jl-wynen changed the base branch from main to git-ignore-lib64 January 21, 2026 15:15
Base automatically changed from git-ignore-lib64 to main January 21, 2026 15:16
@jl-wynen jl-wynen force-pushed the support-large-time-points branch from 12c5887 to a6d2e62 Compare January 22, 2026 08:03
@jl-wynen
Copy link
Copy Markdown
Member Author

jl-wynen commented Jan 22, 2026

After talking with @YooSunYoung I changed the implementation to only write up to the given precision. Please see the updated tests and check if that makes sense. This is consistent with numpy.

Fortunately, I could get rid of the mutex by using the modern std::format API.

@jl-wynen jl-wynen force-pushed the support-large-time-points branch from a6d2e62 to 3a2fc5b Compare January 22, 2026 08:25
YooSunYoung
YooSunYoung approved these changes Jan 22, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@YooSunYoung YooSunYoung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay. Looks nice.

@YooSunYoung
Copy link
Copy Markdown
Member

YooSunYoung commented Jan 22, 2026

@jl-wynen Oh no... it fails to build on windows and macos...

@jl-wynen jl-wynen force-pushed the support-large-time-points branch from f3cf7eb to 8fec440 Compare January 22, 2026 09:57
@jl-wynen
Copy link
Copy Markdown
Member Author

jl-wynen commented Jan 22, 2026

The MacOS build is now failing because we target MacOS 11 (cmake presets contain "CMAKE_OSX_DEPLOYMENT_TARGET": "11.0"). Can we update this or do we still need to support older versions? (@MridulS) Judging by https://en.wikipedia.org/wiki/MacOS versions below 14 are no longer supported anyway.

@MridulS
Copy link
Copy Markdown
Member

MridulS commented Jan 22, 2026

Yeah I think its okay to bump to 14.0, but we do remove support for pre-2018 apple hardware then.

@jl-wynen
Copy link
Copy Markdown
Member Author

jl-wynen commented Jan 22, 2026

That is not great, I don't want to support the business of forcing to replace old hardware all the time. It looks like numpy has wheels for macos 11+ and macos 14+.

@MridulS
Copy link
Copy Markdown
Member

MridulS commented Jan 22, 2026

It looks like numpy has wheels for macos 11+ and macos 14+.

Yes but we are not numpy, if it reduces our maintainence burden I think it's fair to be aggressive in dropping older releases. But yeah no need to drop it if it works.

@jl-wynen jl-wynen mentioned this pull request Jan 23, 2026
Merged
@jl-wynen jl-wynen changed the base branch from main to drop-macos-below-14 January 23, 2026 10:12
Base automatically changed from drop-macos-below-14 to main January 23, 2026 11:44
@jl-wynen jl-wynen force-pushed the support-large-time-points branch from 0183ef3 to c427ed9 Compare January 23, 2026 11:47
@jl-wynen jl-wynen force-pushed the support-large-time-points branch from c427ed9 to fe01f3e Compare January 26, 2026 08:49
@jl-wynen
Copy link
Copy Markdown
Member Author

jl-wynen commented Jan 26, 2026

@MridulS It's passing now. Any more comments?

@MridulS MridulS merged commit f4aa217 into main Jan 27, 2026
4 checks passed
@MridulS MridulS deleted the support-large-time-points branch January 27, 2026 06:36
MridulS pushed a commit to MridulS/scipp that referenced this pull request Feb 9, 2026
@jl-wynen @MridulS
Support time points with unit greater equal days (scipp#3821)
df7e95c 
* Support time points with unit > day

* Only show needed precision

* Write only required digits for date

* Specify duration type explicitly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MridulS MridulS MridulS approved these changes

@YooSunYoung YooSunYoung YooSunYoung approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Switch to C++20

3 participants

@jl-wynen @YooSunYoung @MridulS