Skip to content

Conversation

@jblueh
Copy link
Contributor

@jblueh jblueh commented Mar 15, 2023

Proposed Changes

The python wrapper instantiates drivers directly. To ensure that SU2 is initialized properly, also if used from python, some initialization code is already placed in the driver constructor. This PR moves additional initialization code to the driver constructor.

Related Work

This came up during #1903.

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

@jblueh
Copy link
Contributor Author

jblueh commented Mar 15, 2023

I have a couple of question and suggestions about things I noticed during moving the initialization.

  1. SU2_DOT did not call omp_initialize(), it does now after moving omp_initialize() to the driver constructor. Does it actually run any hybrid AD stuff?
  2. The number of OpenMP threads can be specified for SU2_CFD via the -t parameter, but for SU2_DOT and the python wrapper I saw no way to configure it. Making the number of threads a parameter of the driver constructors and calling omp_set_num_threads in the constructor of CDriverBase seems like a natural way to enable this for all flavours of SU2. Would that be a nice addition?
  3. I couldn't find any python wrapper tests among the parallel AD and hybrid AD tests. Could any of the serial AD python wrapper tests serve as a basis for a parallel AD or hybrid AD one?

@pcarruscag pcarruscag changed the title [WIP] Fix SU2 initialization when using the python wrapper Fix SU2 initialization when using the python wrapper Mar 15, 2023
Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

Thanks 👍 LGTM

@jblueh
Copy link
Contributor Author

jblueh commented Mar 15, 2023

Thanks for your approval @pcarruscag. Could I have your opinion on the questions? E.g., I could add 2. as part of this PR, now that I already looked into it.

@pcarruscag
Copy link
Member

Ah sorry,
SU2_DOT mostly only uses OpenMP in the passive linear system, I'm not sure if @thomasdick Sobolev smoother has some recorded part that involves OpenMP.
For the python wrapper I usually set the number of threads with env var OMP_NUM_THREADS It may be a bit confusing for most users having to specify the number of threads when instantiating the driver 🤔
I think disc_adj_flow would be the easiest to adapt. Those tests print a sensitivity at a particular vertex of a marker to be checked by the regression script, so you would have to make sure that's only printed on one rank (for parallel AD).
disc_adj_fea should also be fine for hybrid AD.

@jblueh
Copy link
Contributor Author

jblueh commented Mar 16, 2023

Thanks for your feedback! I agree, OMP_NUM_THREADS is better than having the number of threads in the constructor. I adapted disc_adj_flow and disc_adj_fea for MPI and added both to parallel AD and hybrid AD tests.

@jblueh jblueh merged commit 316ef59 into develop Mar 17, 2023
@jblueh jblueh deleted the fix_init_functions branch March 17, 2023 14:15
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.

4 participants