Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Event callback slightly different per report #875

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Event callback slightly different per report #875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
554d08e
Event callback slightly different per report
jorblancoa Oct 27, 2022
02654d2
Dont change Binary reports behavior
jorblancoa Oct 28, 2022
4e29f14
Add report time shift only in the first iteration so it doesnt accumu…
jorblancoa Oct 28, 2022
d905729
t shift should be a double
jorblancoa Oct 31, 2022
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
1 change: 1 addition & 0 deletions coreneuron/io/reports/nrnreport.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ struct ReportConfiguration {
int num_gids; // total number of gids
int buffer_size; // hint on buffer size used for this report
std::set<int> target; // list of gids for this report
int report_index; // index of the report
};

void setup_report_engine(double dt_report, double mindelay);
Expand Down
1 change: 1 addition & 0 deletions coreneuron/io/reports/report_configuration_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ std::vector<ReportConfiguration> create_report_configurations(const std::string&
// extra new line: skip
report_conf.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}
report.report_index = report.format == "SONATA" ? i : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
report.report_index = report.format == "SONATA" ? i : 0;
report.report_index = i;

I think we don't need to have this specialised for sonata only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing differences in the binary reports in the CI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm ok!

How many such reports are different? the new logic of "determinist ordering" of report recording seems cleaner right? In that case, I wonder if we should just change the report files.

reports.push_back(report);
}
// read population information for spike report
Expand Down
13 changes: 9 additions & 4 deletions coreneuron/io/reports/report_event.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ ReportEvent::ReportEvent(double dt,
double tstart,
const VarsToReport& filtered_gids,
const char* name,
double report_dt)
double report_dt,
int report_index)
: dt(dt)
, tstart(tstart)
, report_path(name)
, report_dt(report_dt)
, vars_to_report(filtered_gids) {
, vars_to_report(filtered_gids)
, report_t_shift_(1e-6 * report_index) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just to avoid hardcoded factor - what if we replace 1e-6 with dt/1e-3? i.e. whatever will be the value of dt in the future, we can always have this factor 1e3 times lower.

nrn_assert(filtered_gids.size());
step = tstart / dt;
reporting_period = static_cast<int>(report_dt / dt);
Expand Down Expand Up @@ -74,7 +76,7 @@ void ReportEvent::summation_alu(NrnThread* nt) {

/** on deliver, call ReportingLib and setup next event */
void ReportEvent::deliver(double t, NetCvode* nc, NrnThread* nt) {
/* reportinglib is not thread safe */
/* reportinglib is not thread safe */
#pragma omp critical
{
summation_alu(nt);
Expand All @@ -88,8 +90,11 @@ void ReportEvent::deliver(double t, NetCvode* nc, NrnThread* nt) {
gids_to_report.data(),
report_path.data());
#endif
send(t + dt, nc, nt);
// Deterministic event time per report to avoid deadlocks
send(t + dt + report_t_shift_, nc, nt);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's the only way to do it without changing the API of reportinglib? it's a bit ugly that we need to add this artificial 1e-6*report_idx to fix the order.

step++;
// Apply shift only in the first iteration so it doesn't accumulate
report_t_shift_ = 0;
}
}

Expand Down
4 changes: 3 additions & 1 deletion coreneuron/io/reports/report_event.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class ReportEvent: public DiscreteEvent {
double tstart,
const VarsToReport& filtered_gids,
const char* name,
double report_dt);
double report_dt,
int report_index);

/** on deliver, call ReportingLib and setup next event */
void deliver(double t, NetCvode* nc, NrnThread* nt) override;
Expand All @@ -48,6 +49,7 @@ class ReportEvent: public DiscreteEvent {
double step;
std::string report_path;
double report_dt;
double report_t_shift_;
int reporting_period;
std::vector<int> gids_to_report;
double tstart;
Expand Down
3 changes: 2 additions & 1 deletion coreneuron/io/reports/report_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ void ReportHandler::create_report(double dt, double tstop, double delay) {
t,
vars_to_report,
m_report_config.output_path.data(),
m_report_config.report_dt);
m_report_config.report_dt,
m_report_config.report_index);
report_event->send(t, net_cvode_instance, &nt);
m_report_events.push_back(std::move(report_event));
}
Expand Down