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.

Sign up for GitHub

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

Stricter checks on network construction / simulation parameters. #2264

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Stricter checks on network construction / simulation parameters. #2264

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6a5bdc4
Tiny fix for |epoch| < dt.
thorstenhater Apr 2, 2024
4fb5476
Tighten the checks on simulation::run.
thorstenhater Apr 3, 2024
12261a2
Stricter checks, adjust tests.
thorstenhater Apr 3, 2024
2739392
Loosen checks a tiny bit, ensure min_delay is >0.
thorstenhater Apr 3, 2024
0893669
Merge, relax check on t_final, and use isfinite.
thorstenhater Sep 11, 2024
4797b9f
Fix tests.
thorstenhater Sep 20, 2024
b85e8e6
Merge remote-tracking branch 'origin/master' into bug/epoch-lt-dt
thorstenhater Sep 20, 2024
73acec6
Format.
thorstenhater Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions arbor/connection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#pragma once

#include <cstdint>

#include <arbor/common_types.hpp>
#include <arbor/spike.hpp>
#include <arbor/spike_event.hpp>
Expand Down
6 changes: 3 additions & 3 deletions arbor/include/arbor/recipe.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ struct cell_connection_base {

cell_connection_base(L src, cell_local_label_type dst, float w, const U::quantity& d):
source(std::move(src)), target(std::move(dst)), weight(w), delay(d.value_as(U::ms)) {
if (std::isnan(weight)) throw std::out_of_range("Connection weight must be finite.");
if (std::isnan(delay) || delay < 0) throw std::out_of_range("Connection delay must be non-negative and infinite in units of [ms].");
if (std::isnan(weight)) throw std::domain_error("Connection weight must be finite.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Message doesn't match test: should this test be !std::isfinite(weight)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's better, yes.

if (std::isnan(delay) || delay <= 0) throw std::domain_error("Connection delay must be positive, finite, and given in units of [ms].");
Copy link
Contributor

Choose a reason for hiding this comment

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

Inasmuch as we're presuming NaNs work as expected, we could just have the test !(delay > 0) instead of std::isnan(delay) || delay <= 0.

Do we really need to enforce that delay is finite? If so, then the test should include that.

Also, not being familiar (yet) with how LLNL units works, why do we need to specify that the quantity is in milliseconds? Can't we just convert as required or else assert in the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the two issues go hand in hand: (42 * U.ms).value_as(U.mV) == NaN. So, receiving a nan can mean either we got nan * U.ms or an erroneous unit. This is why the message allows for both.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have quite specific exceptions such as arb::bad_connection_source_gid defined in <arbexcept.hpp>; it would be consistent to define some exceptions to throw here that derive from arb::arbor_exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a bit torn here, std::domain_error fits the basic checks for isnan etc well and has the advantage that it plays nice with pybind11 (it'll automatically convert to ValueError).

};

Expand All @@ -69,7 +69,7 @@ struct gap_junction_connection {

gap_junction_connection(cell_global_label_type peer, cell_local_label_type local, double g):
peer(std::move(peer)), local(std::move(local)), weight(g) {
if (std::isnan(weight)) throw std::out_of_range("Gap junction weight must be finite.");
if (std::isnan(weight)) throw std::domain_error("Gap junction weight must be finite.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comments as above: testing finite or testing Nan? We should use an arbor exception.

}
};

Expand Down
13 changes: 8 additions & 5 deletions arbor/simulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "threading/threading.hpp"
#include "util/maputil.hpp"
#include "util/span.hpp"
#include "util/strprintf.hpp"
#include "profile/profiler_macro.hpp"

namespace arb {
Expand Down Expand Up @@ -355,7 +356,7 @@ void simulation_state::reset() {
time_type simulation_state::run(time_type tfinal, time_type dt) {
// Progress simulation to time tfinal, through a series of integration epochs
// of length at most t_interval_. t_interval_ is chosen to be no more than
// than half the network minimum delay.
// than half the network minimum delay and minimally the timestep `dt`.
//
// There are three simulation tasks that can be run partially in parallel:
//
Expand Down Expand Up @@ -394,10 +395,12 @@ time_type simulation_state::run(time_type tfinal, time_type dt) {
// Requires state at end of run(), with epoch_.id==k:
// * U(k) and D(k) have completed.

if (!std::isfinite(tfinal) || tfinal < 0) throw std::domain_error("simulation: tfinal must be finite, positive, and in [ms]");
if (!std::isfinite(dt) || tfinal < 0) throw std::domain_error("simulation: dt must be finite, positive, and in [ms]");

if (tfinal<=epoch_.t1) return epoch_.t1;
// Compare up to picoseconds
time_type eps = 1e-9;
if (!std::isfinite(tfinal) || tfinal < eps) throw std::domain_error("simulation: tfinal must be finite, positive, and in [ms]");
if (epoch_.t1 - tfinal > eps) throw std::domain_error(util::pprintf("simulation: tfinal={}ms is in the past, current time of simulation is {}ms", tfinal, epoch_.t1));
if (!std::isfinite(dt) || dt < eps) throw std::domain_error("simulation: dt must be finite, positive, and in [ms]");
if (dt - t_interval_ > eps) throw std::domain_error(util::pprintf("simulation: dt={}ms is larger than epoch length by {}, chose at most half the minimal connection delay {}ms.", dt, dt - t_interval_, t_interval_));

// Compute following epoch, with max time tfinal.
auto next_epoch = [tfinal](epoch e, time_type interval) -> epoch {
Expand Down
Loading