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

core: replaced utils::strcat_alloc with printf #4069

Merged
merged 5 commits into from
Dec 23, 2020

Conversation

reinaual
Copy link
Contributor

@reinaual reinaual commented Dec 22, 2020

Fixes #4020

Description of changes:

  • replaced manual string char* construct with std::printf
  • introduced verbose parameter to touched tuning functions and interfaces
  • removed utils/strcat_alloc.hpp with unit tests
  • added missing handle_errors call in p3m_gpu_init

double *_r_cut_iL, double *_alpha_L,
double *_accuracy) {
template <bool verbose>
static double p3m_mc_time(const int mesh[3], int cao, double r_cut_iL_min,
Copy link
Member

Choose a reason for hiding this comment

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

Here and elsewhere: I do not immediately see the advantage of using a template parameter instead of a regular parameter. Could you please let me know why this was necessary?

With a template function, there is no guarantee the true and false instantiations will do the same calculation. It is, for example, perfectly legal for someone to inject this snippet of code at line 798:

template <>
double p3m_mc_time<false>(const int mesh[3], int cao, double r_cut_iL_min,
                          double r_cut_iL_max, double *_r_cut_iL,
                          double *_alpha_L, double *_accuracy) {
  return 0.;
}

This will cause the tuning to select random values, which means we should duplicate the mmm1d/p3m/dp3m python tests to make sure verbose=False gives the same results as verbose=True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The template was just personal preference. I dont care, your choice is probably to change it to a regular paramter, i can quickly do that if you want me to.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more maintainable with a regular parameter. This would also allow you to merge p3m_adaptive_tune_core() and p3m_adaptive_tune() back into one function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im working on it, this is an artifact of cython not supporting non-type templates

@jngrad jngrad added this to the Espresso 4.2 milestone Dec 23, 2020
@kodiakhq kodiakhq bot merged commit dc06e0a into espressomd:python Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove strcat_alloc()
2 participants