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

Feature hb #311

Merged
merged 57 commits into from
Oct 5, 2016
Merged

Feature hb #311

merged 57 commits into from
Oct 5, 2016

Conversation

arubino
Copy link
Member

@arubino arubino commented Sep 29, 2016

Hi All,

This is a pull request for the harmonic balance implementation (in the CSpectralDriver). These are the major changes:

  • Full Harmonic Balance implementation working with any set of input frequencies (as opposed to previous Time Spectral implementation)
  • Removed Time-Spectral and generalized with the HB implementation
  • Moved the output from CSpectralDriver to COutput
  • Moved GetnZone and GetnDim from definition_structure to CConfig

Cheers,
Antonio & Sravya

Copy link
Member

@hlkline hlkline left a comment

Choose a reason for hiding this comment

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

I had a couple of comments around the test cases but otherwise this looks like a great addition/update.

@@ -490,7 +490,7 @@ def main():
centrifugal_stage.cfg_dir = "turbomachinery/centrifugal_stage"
centrifugal_stage.cfg_file = "centrifugal_stage.cfg"
centrifugal_stage.test_iter = 100
centrifugal_stage.test_vals = [-10.166364, 1.621172, 2.206476e+01, 5.271075e-01] #last 4 columns
centrifugal_stage.test_vals = [-10.166717, 1.620590, 30.003500, 0.487959] #last 4 columns
Copy link
Member

Choose a reason for hiding this comment

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

Is there an explanation for why the values changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Yes, the quantities for the turbomachinery test case are different now. There is a completely new structure for the turbomachinery performance that will be integrated soon when we will merge the turbomachiney features. After discussing about this with Salvo (that prepared that regression test), we decided that it's OK to just change the residuals for the time being. I can, anyway, have a closer look.

Copy link
Member

Choose a reason for hiding this comment

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

As long as someone running the code will be able to get the same results they got previously it should be fine - if the simulation is run to convergence (ie, more than 100 iterations), do the force coefficients (or other relevant turbomachinery output) converge to the same values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi Heather, thanks a lot for this check because there was indeed a small bug related to the rotating frame! This should be fixed now and I also checked the final solution.

% POISSON_EQUATION)
PHYSICAL_PROBLEM= EULER
%
% Specify turbulent model (NONE, SA, SA_NEG, SST)
KIND_TURB_MODEL= NONE
%
% Mathematical problem (DIRECT, CONTINUOUS_ADJOINT)
% Mathematical problem (DIRECT, ADJOINT, LINEARIZED, ONE_SHOT_ADJOINT)
Copy link
Member

Choose a reason for hiding this comment

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

suggest changing to CONTINUOUS_ADJOINT, which is the newer termonology used;

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the comments Heather. We'll edit the config file and commit the change.

if ((SU2_TYPE::Int(iExtIter) >= 100) && (SU2_TYPE::Int(iExtIter) < 1000)) SPRINTF (buffer, "_00%d.csv", SU2_TYPE::Int(iExtIter));
if ((SU2_TYPE::Int(iExtIter) >= 1000) && (SU2_TYPE::Int(iExtIter) < 10000)) SPRINTF (buffer, "_0%d.csv", SU2_TYPE::Int(iExtIter));
if (SU2_TYPE::Int(iExtIter) >= 10000) SPRINTF (buffer, "_%d.csv", SU2_TYPE::Int(iExtIter));
if (config->GetUnsteady_Simulation() == HARMONIC_BALANCE) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you removed by accident also the extension for the time accurate unsteady simulation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, indeed! We should find a way to have Travis to check the .csv files as well. Thanks for checking this Tim!

#ifdef HAVE_CGNS
#include "cgnslib.h"
#endif

Copy link
Member

@talbring talbring Sep 30, 2016

Choose a reason for hiding this comment

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

This can be removed I think. Sorry, it's correct this way!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey Tim, thanks that is needed for the GetnDim now moved in config_structure.cpp

spectral.cfg_file = "HB.cfg"
spectral.test_iter = 25
spectral.test_vals = [-1.569573, 3.941896, 0.008780, 0.079775] #last 4 columns
spectral.su2_exec = "SU2_CFD"
Copy link
Member

Choose a reason for hiding this comment

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

Should be "parallel_computation.py -f" instead of SU2_CFD, sorry :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! Just pushed the correction.

Copy link
Member

@talbring talbring left a comment

Choose a reason for hiding this comment

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

Thanks for this nice contribution! I am happy now!

@arubino
Copy link
Member Author

arubino commented Oct 3, 2016

Thanks all for the reviews! I would wait for at least another reviewer approval before merging it.

Copy link
Member

@economon economon left a comment

Choose a reason for hiding this comment

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

Overall, looking good. Just one important consideration about changing the CSpectraDriver name.

@@ -413,63 +413,60 @@ class CMultiZoneDriver : public CDriver {

/*!
* \class CSpectralDriver
* \brief Class for driving an iteration of a spectral method problem using multiple zones.
* \brief Class for driving an iteration of harmonic balance method problem using multiple zones.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should also take this chance to rename the child driver class to CHarmonicBalanceDriver or CHBDriver? This would be more consistent with the replacements of "time spectral" with "harmonic balance" throughout the rest of the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Tom. Yes, we actually discussed about this together with Sravya. I changed to CHBDriver now. I used CHBDriver instead of CHarmonicBalanceDriver thinking at the next implementation that would be something like CMultiZoneHB. Let me know what you think about it.

### Harmonic Balance ###
######################################

spectral = TestCase('spectral')
Copy link
Member

Choose a reason for hiding this comment

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

Change the test name to "harmonic_balance" rather than "spectral" for consistency

Copy link
Member Author

@arubino arubino Oct 4, 2016

Choose a reason for hiding this comment

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

Yes, I also changed the old SPECTRAL_METHOD option in the test case config files and in the extended config template with the last commit.

Copy link
Member

@hlkline hlkline left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@economon economon left a comment

Choose a reason for hiding this comment

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

Looks Good!

@economon economon merged commit 6eb7aeb into develop Oct 5, 2016
@arubino
Copy link
Member Author

arubino commented Oct 5, 2016

Thanks All! Deleting the branch now ...

@arubino arubino deleted the feature_HB branch October 5, 2016 07:09
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.

5 participants