Skip to content

Conversation

@HahsFilip
Copy link
Contributor

@HahsFilip HahsFilip commented Jul 28, 2023

Proposed Changes

Give a brief overview of your contribution here in a few sentences.
My changes allow the changing of moving wall velocities when using the py_wrapper.

Related Work

Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.
There has been some previous work, where functionality for changing mesh rotation rates was added.

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • [X ] 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.

@HahsFilip
Copy link
Contributor Author

@pcarruscag This is my first PR, so I hope that at least a few things will be ok :)

@kursatyurt kursatyurt changed the title Ability to change moving walls using the py_wrapper Python interface for updating translation and rotation rates of markers Jul 28, 2023
@@ -0,0 +1,26 @@
import pysu2 # imports the SU2 wrapped module
Copy link
Member

Choose a reason for hiding this comment

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

Thanks 👍 to make a regression test from this you need to add it to parallel_regression.py

Copy link
Member

Choose a reason for hiding this comment

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

Add our license header to this file please (since it's also code).

@HahsFilip
Copy link
Contributor Author

I'm having some trouble getting the regression tests to pass - for some reason there is a result difference in the moving wall tests. I'm not sure how this can be possible, considering I am not changing any values. The convergence seems to be slowed down considerably, but I'm having trouble identifying which function may be causing this.

@pcarruscag
Copy link
Member

Why do you say the convergence is slowed down? It looks like a very small change. Did you run the cases for more iterations?

@HahsFilip
Copy link
Contributor Author

HahsFilip commented Jul 31, 2023

Well, I've noticed the problem, because the unit tests don't pass - cavity and spinning cylinder especially. They exceed the number of iterations, and at the same time there is a result discrepancy. Increasing the number o iterations in serial_regression.py does not help, in fact it makes the delta worse.

@HahsFilip
Copy link
Contributor Author

I'm actually having the same problems running the code on the develop branch....

@pcarruscag
Copy link
Member

Thank can be just due to compilation differences.
Take those two cases and run them until convergence, e.g. 6 orders drop in residuals, then compare the results and if they converge in a similar number of iterations.
If all looks the same we can update the reference values in the test.

@HahsFilip
Copy link
Contributor Author

So you mean I should run the tests to convergence, and compare the results between branches? I wonder why compilation problems could be the culprit, as the tests fail both locally and in the github test. Shouldn't the github test always be compiled the same way?

@pcarruscag
Copy link
Member

You mentioned issues with the develop branch, those are likely due to compilation differences.
Yes run to convergence with both branches and compare please. I expect integrated quantities will be exactly the same.

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.

Hey @HahsFilip do you think you can wrap this up?
Take care of Kursat's comments and update the failing tests since it seem to be just numerical noise (or do you have reason to believe otherwise?)

pcarruscag and others added 2 commits October 26, 2023 13:38
Co-authored-by: Kürşat Yurt <57598663+kursatyurt@users.noreply.github.com>
@pcarruscag
Copy link
Member

You also need to add yourself to the AUTHORS.md file

@HahsFilip
Copy link
Contributor Author

@pcarruscag, sorry for the delay I had to get my life together a bit after the conference :). I think I have the correct changes, I have changed the tests and removed unused lines.

Filip

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.

LGTM

@@ -0,0 +1,26 @@
import pysu2 # imports the SU2 wrapped module
Copy link
Member

Choose a reason for hiding this comment

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

Add our license header to this file please (since it's also code).

Copy link
Member

Choose a reason for hiding this comment

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

You need to revert these changes to submodules, this may be why you see some failing cases

@kursatyurt
Copy link
Contributor

@pcarruscag , @HahsFilip

Do the hybrid tests need an update? Do any of you have any idea about why they are failing?

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.

Because this branch has the wrong versions of various submodules

@kursatyurt
Copy link
Contributor

Because this branch has the wrong versions of various submodules

I see I assumed the last commit reverted all. As it is on a fork we cannot revert 👍🏼

@kursatyurt
Copy link
Contributor

@pcarruscag, is it possible to merge this PR to some temporary branch in the SU2 repo, fix the broken submodules there, and merge it to the main branch? It looks like @HahsFilip is busy. PR is almost complete but cannot be merged. I will do the required fixes.

@pcarruscag pcarruscag merged commit 99141db into su2code:develop Dec 6, 2023
@pcarruscag
Copy link
Member

Thanks @kursatyurt submodules are fixed here 8b89e85
If you can do the other fixes and hook up the regression test it would be great

@kursatyurt
Copy link
Contributor

Thanks @pcarruscag , it looks like standard regression tests are passed without any problem, and ARM tests are problematic, as always.

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.

3 participants