Make Particle distribution score/statistics postprocessors work with multiple particle managers#6908
Conversation
gassmoeller
left a comment
There was a problem hiding this comment.
Yes, the ideas are all correct, but I would suggest modifications to the implementations, see below. Let me know when I should take another look.
| for (unsigned int y=0; y<granularity; ++y) | ||
| const double particles_in_cell_double = static_cast<double>(particles_in_cell); | ||
| const double granularity_double = static_cast<double>(granularity); | ||
| const double ideal_n_particles_per_bucket = particles_in_cell_double/(Utilities::fixed_power<dim>(granularity_double)); |
There was a problem hiding this comment.
you can compute granularity_double and ideal_n_particles_per_bucket outside of the loop over all cells, that will save compute time.
There was a problem hiding this comment.
I agree for granularity_double, but I think that ideal_n_particles_per_bucket depends on the number of particles in each cell which could vary depending on the cell. For example in 2D with a granularity of 2 we would have 4 buckets per cell. Then a cell with 4 particle would want 1 particle per bucket, while a cell with 8 particles would want 2 particles per bucket? Or were you thinking of calculating the ideal particles per bucket from the minimum or maximum # of allowed particles per cell?
There was a problem hiding this comment.
Correct, I wasnt paying attention.
| The "buckets_ideal" table contains doubles because the ideal | ||
| number of particles per bucket will not always be an integer. | ||
| */ | ||
| Table<dim,double> buckets_ideal; |
There was a problem hiding this comment.
I didnt catch this before, but it seems you dont need buckets_ideal in this postprocessor at all. You fill all entries of buckets_ideal with ideal_n_particles_per_bucket and then later use the entries. That means you can just use ideal_n_particles_per_bucket in line 104 and 111 and remove buckets_ideal altogether.
| // get final values for min and max score from all processors | ||
| const double global_max_score = Utilities::MPI::max (local_max_score, this->get_mpi_communicator()); | ||
| const double global_min_score = Utilities::MPI::min (local_min_score, this->get_mpi_communicator()); | ||
| if (this->n_particle_managers()==1) |
There was a problem hiding this comment.
I dont think you want the if condition here. See my suggestion below.
| std::ostringstream output; | ||
| output << global_min_score << '/' << mean_and_standard_deviation.first << '/' << global_max_score << '/' << mean_and_standard_deviation.second; | ||
| return std::pair<std::string, std::string> ("Particle Distribution Score (min/avg/max/stdev): ", | ||
| output.str()); |
There was a problem hiding this comment.
I would generalize this to the case of 1 particle handler as well, that will simplify your code. I.e. remove the whole if branch above, and change this to:
| output.str()); | |
| std::ostringstream output; | |
| for (unsigned int particle_manager_index = 0; particle_manager_index < this->n_particle_managers(); ++particle_manager_index) | |
| { | |
| // Calculate the mean and standard deviation of cell scores across all processors | |
| const std::pair<double, double> mean_and_standard_deviation = | |
| Utilities::MPI::mean_and_standard_deviation(cell_scores[particle_manager_index].begin(), | |
| cell_scores[particle_manager_index].end(), | |
| this->get_mpi_communicator()); | |
| // get final values for min and max score from all processors | |
| const double global_max_score = Utilities::MPI::max (local_max_scores[particle_manager_index], this->get_mpi_communicator()); | |
| const double global_min_score = Utilities::MPI::min (local_min_scores[particle_manager_index], this->get_mpi_communicator()); | |
| // write to statistics file for all particle managers | |
| const std::string particle_manager_index_prefix = (particle_manager_index == 0) ? "" : "Particles " + std::to_string(particle_manager_index+1) + ": "; | |
| statistics.add_value (particle_manager_index_prefix+"Minimal particle distribution score: ", global_min_score); | |
| statistics.add_value (particle_manager_index_prefix+"Average particle distribution score: ", mean_and_standard_deviation.first); | |
| statistics.add_value (particle_manager_index_prefix+"Maximal particle distribution score: ", global_max_score); | |
| statistics.add_value (particle_manager_index_prefix+"Cell Score Standard Deviation: ", mean_and_standard_deviation.second); | |
| // write screen output for the first particle manager | |
| if (particle_manager_index == 0) | |
| output << global_min_score << '/' << mean_and_standard_deviation.first << '/' << global_max_score << '/' << mean_and_standard_deviation.second; | |
| } | |
| return std::pair<std::string, std::string> ("Particle distribution score min/avg/max/stdev: ", | |
| output.str()); |
Also leave the old output capitalization and wording of the return string, it is more consistent with our other postprocessors. (It is the particle distribution statistics postprocessor that is not following conventions and should be changed, but not in this PR).
There was a problem hiding this comment.
Thanks for letting me know, I will open another PR correcting the wording of particle distribution statistics after this.
| return std::pair<std::string, std::string> ("Particle Distribution Stats (stddev min/mean/max, mean min/max, absolute min/max):", | ||
| output.str()); | ||
| } | ||
| else |
There was a problem hiding this comment.
Same comment as above. Remove the if-branch. Modify the else-branch to output the statistics file and the screen output as before for particle manager 0. Do not output screen output for particle manager >0, and prefix the statistics columns for particle manager >0 with "Particles x:" where x is particle manager index +1 (this is so that it is consistent with the numbering we use in the input file).
| Writing particle output: output-benchmark_particle_pdf_removal/particles/particles-00000 | ||
| Particle distribution score min/avg/max/stdev: 0/0/0/0 | ||
| Particle Distribution Stats (stddev min/mean/max, mean min/max, absolute min/max): 0/0/2.22507e-308, 0.0592697/0.0592697, 0.0454545/0.1 | ||
| Particle Distribution Score (min/avg/max/stdev): 0/0/0/0 |
There was a problem hiding this comment.
with my suggested changed above, you shouldnt see any test changes.
There was a problem hiding this comment.
This file needs to be named screen-output, not screen-output.txt. It is a leftover from very early in ASPECT development.
gassmoeller
left a comment
There was a problem hiding this comment.
Close, but the test results are still changing. Check what happens if you include my suggestions.
|
|
||
| double actual_error_squared = 0; | ||
| for (unsigned int x=0; x<granularity; ++x) | ||
| unsigned int particles_in_cell = this->get_particle_manager(particle_manager_index).get_particle_handler().n_particles_in_cell(cell); |
There was a problem hiding this comment.
| unsigned int particles_in_cell = this->get_particle_manager(particle_manager_index).get_particle_handler().n_particles_in_cell(cell); | |
| const unsigned int particles_in_cell = this->get_particle_manager(particle_manager_index).get_particle_handler().n_particles_in_cell(cell); |
| TableIndices<dim> bucket_sizes; | ||
| for (unsigned int i=0; i<dim; ++i) | ||
| bucket_sizes[i] = granularity; |
There was a problem hiding this comment.
Are bucket_sizes still used? If not, they can be removed.
| // These need to be vectors to account for multiple particle managers. | ||
| std::vector<double> local_min_scores(this->n_particle_managers(),std::numeric_limits<double>::max()); | ||
| std::vector<double> local_max_scores(this->n_particle_managers(),0); | ||
| std::vector<std::vector<double>> cell_scores(this->n_particle_managers()); |
There was a problem hiding this comment.
While we are touching this line anyway: Can you add the following:
| std::vector<std::vector<double>> cell_scores(this->n_particle_managers()); | |
| std::vector<std::vector<double>> cell_scores(this->n_particle_managers()); | |
| for (auto &scores: cell_scores) | |
| scores.reserve(this->get_triangulation().n_active_cells()); |
This will avoid unnecessary memory allocation and copy operations.
| for (unsigned int y=0; y<granularity; ++y) | ||
| const double particles_in_cell_double = static_cast<double>(particles_in_cell); | ||
| const double granularity_double = static_cast<double>(granularity); | ||
| const double ideal_n_particles_per_bucket = particles_in_cell_double/(Utilities::fixed_power<dim>(granularity_double)); |
There was a problem hiding this comment.
Correct, I wasnt paying attention.
| if (particle_manager_index == 0) | ||
| { | ||
| output << global_standard_deviation_min << "/" << global_standard_deviation_mean << "/" << global_standard_deviation_max << ", " | ||
| << global_function_min_mean << "/" << global_function_max_mean << ", " << global_function_min_min << "/" << global_function_max_max << ", "; |
There was a problem hiding this comment.
| << global_function_min_mean << "/" << global_function_max_mean << ", " << global_function_min_min << "/" << global_function_max_max << ", "; | |
| << global_function_min_mean << "/" << global_function_max_mean << ", " << global_function_min_min << "/" << global_function_max_max; |
| << global_function_min_mean << "/" << global_function_max_mean << ", " << global_function_min_min << "/" << global_function_max_max << ", "; | ||
| } | ||
| } | ||
| return std::pair<std::string, std::string> ("Particle Distribution Stats (stddev min/mean/max, mean min/max, absolute min/max): ", |
There was a problem hiding this comment.
| return std::pair<std::string, std::string> ("Particle Distribution Stats (stddev min/mean/max, mean min/max, absolute min/max): ", | |
| return std::pair<std::string, std::string> ("Particle Distribution Stats (stddev min/mean/max, mean min/max, absolute min/max):", |
|
I think this is ready for another review. |
|
/rebuild |
gassmoeller
left a comment
There was a problem hiding this comment.
Thanks! Except for my one question about the test output this is ready. Please have a look and let me know what you find.
| Writing graphical output: output-benchmark_particle_pdf_removal/solution/solution-00000 | ||
| Writing particle output: output-benchmark_particle_pdf_removal/particles/particles-00000 | ||
| Particle distribution score min/avg/max/stdev: 0/0/0/0 | ||
| Particle Distribution Stats (stddev min/mean/max, mean min/max, absolute min/max): 0/0/0, 0.0592697/0.0592697, 0.0454545/0.1, |
There was a problem hiding this comment.
I dont understand why there is a trailing comma in this line. I dont see it written in the code, but the tester seems to accept it. Is it a leftover from earlier versions of this PR? Do you see where it is coming from?
Before this pull request the Particle distribution score and Particle distribution statistics postprocessors would calculate values across multiple particle managers (if there were multiple particle managers used in the model). This would make the results of the postprocessors inaccurate since the particle managers take care of particles separately from one another.
Now each postprocessor behaves as before if there is only one particle manager, but if there are more than one the postprocessors will calculate and report their statistics separately for each particle manager.