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

Fix BCs and geometry for pinball #195

Merged
merged 1 commit into from
Oct 5, 2024
Merged

Fix BCs and geometry for pinball #195

merged 1 commit into from
Oct 5, 2024

Conversation

jcallaham
Copy link
Collaborator

Comparing with the recent TCFD article describing this flow configuration, this PR makes the following changes:

  • Sign error in Dirichlet BC for solid-body rotation
  • Increase actuator limits on Pinball
  • Decrease "tau" value for first-order filter on Pinball
  • Fix the cylinder locations (x-locations were slightly wrong)
  • Rename the "fine" mesh to "medium" and make this the default
  • Add a new "fine" mesh with domain extent matching the paper (but with around 3x more total mesh elements)

The paper also uses a prescribed velocity of (1.0, 0.0) for the free-stream boundary conditions, but I'm leaving as symmetry here. I doubt it makes much of a difference in the dynamics, and I expect symmetry BCs will work better on smaller domains.

C.C. @cl126162 @smokbel @ReHoss

@jcallaham jcallaham self-assigned this Oct 3, 2024
@jcallaham jcallaham merged commit 5b691fd into main Oct 5, 2024
6 checks passed
@ReHoss
Copy link

ReHoss commented Oct 5, 2024

Hello @jcallaham

Thank you for the update.

Comparing with the recent TCFD article describing this flow configuration, this PR makes the following changes:

  • Sign error in Dirichlet BC for solid-body rotation

Is it something critical, or this does not substantially change results?

  • Increase actuator limits on Pinball

I know that the implementation is inspired by the seminal papers on Pinball.
Why is this increase necessary? Is it solely to match the TCFD article?

  • Decrease "tau" value for first-order filter on Pinball
  • Fix the cylinder locations (x-locations were slightly wrong)

Wrong w.r.t. to the recent TCFD paper? But right with respect to the legacy papers?

I am totally fine with the update, I am just curious about its purpose:)

Thank you very much!
Best,

@jcallaham
Copy link
Collaborator Author

jcallaham commented Oct 5, 2024 via email

@ReHoss
Copy link

ReHoss commented Oct 5, 2024

No worries! and thank you for the details, nothing critical from my side. I might come back with questions later on...

By the way, were you able to find a control solution with this sign error on the Pinball? Could this sign problem only affect the signal sign but not the information it contains? @jcallaham

Best,

@jcallaham
Copy link
Collaborator Author

jcallaham commented Oct 5, 2024 via email

@ReHoss
Copy link

ReHoss commented Oct 27, 2024

Hi @jcallaham ,

I have several questions or remarks regarding this update:

  • Suppose that we ignore the slight coordinate change on the Pinball.
    This PR only impacts the steady flow computation of the Pinball and the RotaryCynlinder since it modifies self.bcu_actuation then their respective collect_bcu methods used by NewtonSolver.

  • Before this PR, I computed some steady states and some snapshots of "natural flow" stepping the flows (Cylinder, Pinball and Cavity) with a zero actuation input (with the step method). Ignoring the fix on the boundary condition, the impact of the modification of TAU value should only modify actuated flows? In the case where zero actions are passed, this change should not have any effect on a (not actuated dynamics)?

  • Does the RotaryCylinder object represent the setup where the actuation is a rotation of the physical cylinder while the Cylinder implements a blowing and suction setting?

  • Regarding the actuator limits, for similar blowing and suction schemes we have now two different default maximum values (MAX_ACTUATOR = 10 for the pinball and MAX_ACTUATOR = 0.5 * np.pi for the cylinder).
    I think MAX_ACTUATOR should be equal for all environments (Cylinder and Pinball) with respect to their actuation mode (blowing and suction vs. rotation).

  • Does the choice of MAX_ACTUATOR = 0.5 * np.pi for a rotary cylinder means the cyclinder rotates for at most 45 degrees in both directions ? Would increasing this value to 10 means the cylinder can rotate with an angle greater than $2\pi = 360$ degrees ?

@ReHoss ReHoss mentioned this pull request Oct 27, 2024
2 tasks
@ReHoss
Copy link

ReHoss commented Nov 19, 2024

Hello @jcallaham , could u enlight me on the questions above:)?

@jcallaham
Copy link
Collaborator Author

Oh sorry, I forgot I never got back to you.

  • Suppose that we ignore the slight coordinate change on the Pinball.
    This PR only impacts the steady flow computation of the Pinball and the RotaryCynlinder since it modifies self.bcu_actuation then their respective collect_bcu methods used by NewtonSolver.
  • Before this PR, I computed some steady states and some snapshots of "natural flow" stepping the flows (Cylinder, Pinball and Cavity) with a zero actuation input (with the step method). Ignoring the fix on the boundary condition, the impact of the modification of TAU value should only modify actuated flows? In the case where zero actions are passed, this change should not have any effect on a (not actuated dynamics)?

Yes, that's right - if no actuation is used then there's no change from any of this

  • Does the RotaryCylinder object represent the setup where the actuation is a rotation of the physical cylinder while the Cylinder implements a blowing and suction setting?

Also correct. RotaryCylinder is the same as the typical Pinball setup, but the regular Cylinder matches a couple of references looking into RL and such

  • Regarding the actuator limits, for similar blowing and suction schemes we have now two different default maximum values (MAX_ACTUATOR = 10 for the pinball and MAX_ACTUATOR = 0.5 * np.pi for the cylinder).
    I think MAX_ACTUATOR should be equal for all environments (Cylinder and Pinball) with respect to their actuation mode (blowing and suction vs. rotation).

The only environment that uses blowing/suction is the Cylinder, but yes I agree that the rotary limits (RotaryCylinder vs Pinball) might as well match

  • Does the choice of MAX_ACTUATOR = 0.5 * np.pi for a rotary cylinder means the cyclinder rotates for at most 45 degrees in both directions ? Would increasing this value to 10 means the cylinder can rotate with an angle greater than 2π = 360 degrees ?

The actuation here is the angular velocity, not the absolute angle. So 10 would be 10 rad/s

@ReHoss
Copy link

ReHoss commented Nov 19, 2024

Thank you very much @jcallaham

  • Regarding the actuator limits, for similar blowing and suction schemes we have now two different default maximum values (MAX_ACTUATOR = 10 for the pinball and MAX_ACTUATOR = 0.5 * np.pi for the cylinder).
    I think MAX_ACTUATOR should be equal for all environments (Cylinder and Pinball) with respect to their actuation mode (blowing and suction vs. rotation).

The only environment that uses blowing/suction is the Cylinder, but yes I agree that the rotary limits (RotaryCylinder vs Pinball) might as well match

Consequently, do you recommend setting MAX_ACTUATOR = 10 for the cylinder, too?

Secondly, does the new cylinder location (x-locations) make the previous meshes incompatible (I guess yes)?

@jcallaham
Copy link
Collaborator Author

Consequently, do you recommend setting MAX_ACTUATOR = 10 for the cylinder, too?

For the RotaryCylinder yes we should probably change it in the code. For your own purposes I would say feel free to set it to whatever you want - the only reason it's there at all is to help provide some stability for the early stages of RL training.

Secondly, does the new cylinder location (x-locations) make the previous meshes incompatible (I guess yes)?

Unfortunately yes, the meshes will be different now. Sorry, that's totally my fault - no idea why I got the locations wrong in the first place. I don't think it would change anything qualitatively but I can't say for sure.

@ReHoss
Copy link

ReHoss commented Nov 20, 2024

Unfortunately yes, the meshes will be different now. Sorry, that's totally my fault - no idea why I got the locations wrong in the first place. I don't think it would change anything qualitatively but I can't say for sure.

Thank you very much!:) and no worries I was just asking for my understanding. I am still working with coarse meshes anyway!

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.

3 participants