Skip to content

Commit

Permalink
Refactor discretization. (#2415)
Browse files Browse the repository at this point in the history
This PR rectifies a long-standing issue since the introduction of
`decor`:
CV discretization was treated as a defaultable value instead of a cell
property, along with bio-physical properties.

This lead to some awkward facts:
- Intermingling of bio-physics and numerical settings
- Discretization is treated like a default without override.
- Discretization cannot be changed once set, e.g. if read from `.acc`,
requiring the copy+update of a decor instead

Thus:
- Discretization can now be set during `cable_cell` construction
- It can be read and updated later
- Refactor `cv_policy` to use type erasure like other, similar
constructions, e.g. schedules.
- Hide internal `cv_policy` constructors.

⚠️ Breaks `.acc` by upgrading from 0.9 to 0.10 ⚠️ 

Closes #1408

---------

Co-authored-by: boeschf <[email protected]>
  • Loading branch information
thorstenhater and boeschf authored Oct 18, 2024
1 parent 7d1f82e commit 76120d1
Show file tree
Hide file tree
Showing 69 changed files with 689 additions and 692 deletions.
8 changes: 3 additions & 5 deletions .github/workflows/sanitize.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,16 @@ permissions:
jobs:
build:
name: "Sanitize"
runs-on: ubuntu-22.04
runs-on: ubuntu-24.04
strategy:
fail-fast: false
matrix:
name: ["Sanitize"]
sanitizer: ["address", "undefined", "thread"]
simd: ["ON", "OFF"]
env:
CC: clang-14
CXX: clang++-14
CC: clang-18
CXX: clang++-18
ASAN_OPTIONS: detect_leaks=1
steps:
- name: Get build dependencies
Expand All @@ -43,8 +43,6 @@ jobs:
uses: actions/checkout@v4
with:
submodules: recursive
- name: Update pip
run: python -m pip install --upgrade pip
# figure out vector extensions for ccache key
- name: Check vector extensions
run: |
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/test-matrix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ jobs:
- {
name: "Linux Min Clang",
os: "ubuntu-22.04",
cc: "clang-12",
cxx: "clang++-12",
cc: "clang-13",
cxx: "clang++-13",
py: "3.9",
cmake: "3.19.x",
mpi: "ON",
Expand Down
16 changes: 15 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ install(FILES mechanisms/BuildModules.cmake DESTINATION ${ARB_INSTALL_DATADIR})

# First make ourselves less chatty
set(_saved_CMAKE_MESSAGE_LOG_LEVEL ${CMAKE_MESSAGE_LOG_LEVEL})
set(CMAKE_MESSAGE_LOG_LEVEL WARNING)
set(CMAKE_MESSAGE_LOG_LEVEL STATUS)

# in the event we can find hwloc, just add it
find_package(hwloc QUIET)
Expand Down Expand Up @@ -341,6 +341,8 @@ CPMAddPackage(NAME fmt
VERSION 10.0.0
GIT_TAG 10.0.0)

add_library(ext-gtest INTERFACE)
add_library(ext-bench INTERFACE)
if (BUILD_TESTING)
CPMAddPackage(NAME benchmark
GITHUB_REPOSITORY google/benchmark
Expand All @@ -351,6 +353,18 @@ if (BUILD_TESTING)
GIT_TAG release-1.12.1
VERSION 1.12.1
OPTIONS "INSTALL_GTEST OFF" "BUILD_GMOCK OFF")
if(benchmark_ADDED)
target_link_libraries(ext-bench INTERFACE benchmark)
else()
find_package(benchmark REQUIRED)
target_link_libraries(ext-bench INTERFACE benchmark::benchmark)
endif()
if(googletest_ADDED)
target_link_libraries(ext-gtest INTERFACE )
else()
find_package(googletest REQUIRED)
target_link_libraries(ext-gtest INTERFACE gtest gtest_main)
endif()
endif()

CPMAddPackage(NAME units
Expand Down
18 changes: 13 additions & 5 deletions arbor/cable_cell.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,22 @@ struct cable_cell_impl {
// The decorations on the cell.
decor decorations;

// Discretization
std::optional<cv_policy> discretization_;

// The placeable label to lid_range map
dynamic_typed_map<constant_type<std::unordered_multimap<hash_type, lid_range>>::type> labeled_lid_ranges;

cable_cell_impl(const arb::morphology& m, const label_dict& labels, const decor& decorations):
cable_cell_impl(const arb::morphology& m, const label_dict& labels, const decor& decorations, const std::optional<cv_policy>& cvp):
provider(m, labels),
dictionary(labels),
decorations(decorations)
decorations(decorations),
discretization_{cvp}
{
init();
}

cable_cell_impl(): cable_cell_impl({},{},{}) {}
cable_cell_impl(): cable_cell_impl({}, {}, {}, {}) {}

cable_cell_impl(const cable_cell_impl& other) = default;

Expand Down Expand Up @@ -203,6 +207,10 @@ struct cable_cell_impl {
}
};

const std::optional<cv_policy>& cable_cell::discretization() const { return impl_->discretization_; }
void cable_cell::discretization(cv_policy cvp) { impl_->discretization_ = std::move(cvp); }


using impl_ptr = std::unique_ptr<cable_cell_impl, void (*)(cable_cell_impl*)>;
impl_ptr make_impl(cable_cell_impl* c) {
return impl_ptr(c, [](cable_cell_impl* p){delete p;});
Expand Down Expand Up @@ -232,8 +240,8 @@ void cable_cell_impl::init() {
}
}

cable_cell::cable_cell(const arb::morphology& m, const decor& decorations, const label_dict& dictionary):
impl_(make_impl(new cable_cell_impl(m, dictionary, decorations)))
cable_cell::cable_cell(const arb::morphology& m, const decor& decorations, const label_dict& dictionary, const std::optional<cv_policy>& cvp):
impl_(make_impl(new cable_cell_impl(m, dictionary, decorations, cvp)))
{}

cable_cell::cable_cell(): impl_(make_impl(new cable_cell_impl())) {}
Expand Down
8 changes: 0 additions & 8 deletions arbor/cable_cell_param.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,6 @@ std::vector<defaultable> cable_cell_parameter_set::serialize() const {
for (const auto& [name, mech]: reversal_potential_method) {
D.push_back(ion_reversal_potential_method{name, mech});
}

if (discretization) {
D.push_back(*discretization);
}

return D;
}

Expand Down Expand Up @@ -163,9 +158,6 @@ decor& decor::set_default(defaultable what) {
else if constexpr (std::is_same_v<ion_reversal_potential_method, T>) {
defaults_.reversal_potential_method[p.ion] = p.method;
}
else if constexpr (std::is_same_v<cv_policy, T>) {
defaults_.discretization = std::forward<cv_policy>(p);
}
else if constexpr (std::is_same_v<ion_diffusivity, T>) {
if (p.scale.type() != iexpr_type::scalar) throw cable_cell_error{"Default values cannot have a scale."};
auto s = p.scale.get_scalar();
Expand Down
Loading

0 comments on commit 76120d1

Please sign in to comment.