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

Conversation

jorblancoa
Copy link
Contributor

Description

Enforce deterministic report events

@jorblancoa jorblancoa requested a review from pramodk October 27, 2022 13:09
@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #81113 (:no_entry:) have been uploaded here!

Status and direct links:

@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #81258 (:no_entry:) have been uploaded here!

Status and direct links:

@jorblancoa
Copy link
Contributor Author

still issues when launching with libsonata-report master
https://bbpgitlab.epfl.ch/hpc/sim/blueconfigs/-/jobs/409163

@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #81516 (:white_check_mark:) have been uploaded here!

Status and direct links:

@@ -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.

Copy link
Collaborator

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Some minor comments otherwise LGTM

@@ -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.

: 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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants