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

Stricter checks on network construction / simulation parameters. #2264

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
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
19 changes: 10 additions & 9 deletions arbor/simulation.cpp
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 @@ -394,12 +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;

auto epoch_length = std::max(t_interval_, dt);
// 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 Expand Up @@ -487,8 +488,8 @@ time_type simulation_state::run(time_type tfinal, time_type dt) {
};

epoch prev = epoch_;
epoch current = next_epoch(prev, epoch_length);
epoch next = next_epoch(current, epoch_length);
epoch current = next_epoch(prev, t_interval_);
epoch next = next_epoch(current, t_interval_);

if (epoch_callback_) epoch_callback_(current.t0, tfinal);

Expand All @@ -509,7 +510,7 @@ time_type simulation_state::run(time_type tfinal, time_type dt) {
for (;;) {
prev = current;
current = next;
next = next_epoch(next, epoch_length);
next = next_epoch(next, t_interval_);
if (next.empty()) break;

g.run([&]() { exchange(prev); enqueue(next); });
Expand Down
Loading