-
Notifications
You must be signed in to change notification settings - Fork 201
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
added a workaround for issue 4604 #4631
Conversation
if (is_last_flush_to_step) { | ||
SetupPos(currSpecies, NewParticleVectorSize, isBTD); // workaround for issue 4604 | ||
SetConstParticleRecordsEDPIC(currSpecies, NewParticleVectorSize, charge, mass); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this should go exactly like this here.
What do you think about this?
if (is_last_flush_to_step) { | |
SetupPos(currSpecies, NewParticleVectorSize, isBTD); // workaround for issue 4604 | |
SetConstParticleRecordsEDPIC(currSpecies, NewParticleVectorSize, charge, mass); | |
} | |
if (is_last_flush_and_never_particles) { | |
SetupPos(currSpecies, NewParticleVectorSize, isBTD); | |
} | |
if (is_last_flush_to_step) { | |
SetConstParticleRecordsEDPIC(currSpecies, NewParticleVectorSize, charge, mass); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly, this is exactly what is done a few lines earlier, isn't it? Why is that not called, I wonder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the values of num_dump_particles
and ParticleFlushOffset
for this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
num_dump_particles=0
ParticleFlushOffset=1000
so is_last_flush_and_never_particles (= is_last_flush_to_step && num_dump_particles == 0 && ParticleFlushOffset == 0) will be false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, got it.
If ParticleFlushOffset > 0
that means at some point, at least one rank had particles to write.
We need to check if maybe in the call where we write particles (earlier), we have a logic mistake that forgets to call SetupPos
collectively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe we filter twice and forget num_fraction
in one case? Let's take a look where num_fraction
is applied and if the counter is done correctly afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the offsets are set in the BTD object, and when it calls flush(), the filters are then applied in the underlying flush formats during copyParticles(). Unlike other filters, the random filter affected the offsets of the caller BTD object.
I think the workaround is the easy and harmless. Otherwise, have to collect the particles written from all processes and update the offset in BTD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the workaround is the easy and harmless.
I would rather like to fix the underlying issue in the BTD offset counter. We need consistent meta data.
cc @RevathiJambunathan :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it makes sense, can apply the random filter before BTD calls flush(). It will make things cleaning and easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we probably need to add this to:
- full diags
- BTD
- scraping
Not sure why it fails during "copying files to run directory..." |
Fixing the broken meta-data in #4657 :) |
Guard against BTD case:
See issue #4604