Conversation
|
@jl-wynen just for a sanity check could you also trigger conda and wheel builds from the weekly github action in this PR? |
jl-wynen
force-pushed
the
default-units-namespace
branch
from
4f7c897 to
cc7fdcb
Compare
lib/cmake/scipp-conan.cmake
Outdated
| LLNL-Units:fPIC=True | ||
| LLNL-Units:base_type=uint64_t | ||
| LLNL-Units:namespace=llnl::units | ||
| LLNL-Units:namespace=units |
There was a problem hiding this comment.
Wait you set it here explicitly, but at llnl-units/conanfile.py you don't seem to process this option? I'd remove this line here and rely on upstream to maintain the same default namespace.
There was a problem hiding this comment.
Good catch! I removed it.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the project to use the default LLNL::Units namespace alias (sc_units) throughout the C++ core and build scripts.
- Replaces all occurrences of
units::Unit,units::one, etc. withsc_units::Unit,sc_units::one. - Updates CMake and Conan build recipes to drop custom namespace overrides.
- Adjusts benchmarks and utility headers to reference
sc_unitsconsistently.
Reviewed Changes
Copilot reviewed 174 out of 174 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/core/include/scipp/core/element/rebin.h | Updated lambda signatures to use sc_units::Unit |
| lib/core/include/scipp/core/element/math.h | Swapped units::Unit for sc_units::Unit in math |
| lib/core/include/scipp/core/element/map_to_bins.h | Updated bin lambdas to use sc_units::Unit |
| lib/core/include/scipp/core/element/logical.h | Refactored logical ops to sc_units::Unit |
| lib/core/include/scipp/core/element/histogram.h | Switched histogram units to sc_units::Unit |
| lib/core/include/scipp/core/element/geometric_operations.h | Changed geometry ops to sc_units::Unit |
| lib/core/include/scipp/core/element/event_operations.h | Updated event ops to sc_units::Unit |
| lib/core/include/scipp/core/element/creation.h | Updated special_like to sc_units::Unit |
| lib/core/include/scipp/core/element/comparison.h | Replaced units::none with sc_units::none |
| lib/core/include/scipp/core/element/bin_detail.h | Updated subbin operations to sc_units::Unit |
| lib/core/include/scipp/core/element/bin.h | Revised binning ops to sc_units::Unit |
| lib/core/include/scipp/core/element/arithmetic.h | Updated arithmetic ops to sc_units::Unit |
| lib/core/include/scipp/core/array_to_string.h | Updated optional unit type to sc_units::Unit |
| lib/cmake/scipp-functions.cmake | Updated inline CMake test to use sc_units::rad |
| lib/cmake/scipp-conan.cmake | Removed custom LLNL-Units namespace option |
| lib/cmake/.conan-recipes/llnl-units/conanfile.py | Cleaned up custom namespace overrides |
| lib/benchmark/variable_benchmark.cpp | Changed benchmarks to use sc_units::Unit |
| lib/benchmark/histogram_benchmark.cpp | Updated hist. bench to sc_units::one |
| lib/benchmark/groupby_benchmark.cpp | Updated groupby bench to sc_units::one |
| lib/benchmark/bin_benchmark.cpp | Revised bin bench to broadcast sc_units::one |
Comments suppressed due to low confidence (2)
lib/core/include/scipp/core/element/rebin.h:65
- [nitpick] Consider adding a file-local alias (e.g.,
using Unit = sc_units::Unit;) at the top of this header to reduce repeated long qualifier usage and simplify future renames.
[](const sc_units::Unit &target_edges, const sc_units::Unit &data, const sc_units::Unit &edges) {
lib/cmake/.conan-recipes/llnl-units/conanfile.py:65
- [nitpick] Cleanup or remove any leftover commented code or references to the
UNITS_NAMESPACEoption since namespace overrides are no longer supported.
# Tools.replace_in_file(...) lines removed
|
Thanks! Is there any intention to create a new release with this? I tend towards adding |
|
Yes, I was going to make a release soon. Do you require the other PRs to make a package? That is, should we wait for them or are you happy with the current state? |
|
It'd be nice if my |
Fixes #3705
Supersedes part of #3697
I renamed our namespace to
scipp::sc_unitsso that we can still use a shortsc_units::minstead of having to spell outscipp::units::m. But that is only relevant in C++. So I am open to changing this to keep our original namespace name.