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

Issue 87 #92

Merged
merged 3 commits into from
Jun 21, 2021
Merged

Issue 87 #92

merged 3 commits into from
Jun 21, 2021

Conversation

rtobar
Copy link

@rtobar rtobar commented Jun 21, 2021

This PR addresses #87. It has been shown that these changes fix the original problem by two separate parties, and this PR will trigger yet another set of compilations and executions that will further confirm there are no newly introduce bugs.

rtobar added 3 commits June 15, 2021 17:05
The different routines exchanging extra information (gas, start, BH and
dark matter) during particle MPI exchange pass around two buffers,
indices and property data, whose sizes are related: for N indices there
are N * M properties. However most of the routines were flawed because
they allocated a property reception array of N elements, leading to
memory corruption.

Additionally, this problem affected all these routines (except one)
because the code performing these MPI operations was duplicated in all
of them.

This commit fixes these two problems in one go. Firstly, it adds a new
function to perform the data exchange that is then called by the four
original routines. Secondly, the new centralised version of the data
exchange code correctly sizes the input buffers to avoid memory
corruption.

This commit addresses the issue reported in #87. A similar situation had
been reported in #54, but at the time I hadn't realised this was a
problem affecting several similarly-looking functions, and therefore the
fix at the time (082ff68) was constrained only to the routine where the
problem was originally reported, leaving the rest with the problem still
to be fixed.

Signed-off-by: Rodrigo Tobar <[email protected]>
This was a write-only variable so there's no need to keep it around.

Signed-off-by: Rodrigo Tobar <[email protected]>
@icrar-velociraptor-bot
Copy link

3d-run

Relevant configuration parameters full file:

Parameter Value
Bound_halos 0
Physical_linking_length 0.10
FoF_Field_search_type 5

R_200crit v/s Mass_200crit:

R_200mean v/s Mass_200mean:

Mass_200crit histogram:

Mass_200mean histogram:

R_200crit histogram:

R_200mean histogram:

6d-run

Relevant configuration parameters full file:

Parameter Value
Bound_halos 0
Physical_linking_length 0.10
FoF_Field_search_type 3
Halo_6D_linking_length_factor 1.0
Halo_6D_vel_linking_length_factor 1.25

R_200crit v/s Mass_200crit:

R_200mean v/s Mass_200mean:

Mass_200crit histogram:

Mass_200mean histogram:

R_200crit histogram:

R_200mean histogram:

@rtobar rtobar merged commit 91a83a4 into master Jun 21, 2021
@rtobar rtobar deleted the issue-87 branch June 21, 2021 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants