Skip to content

Upgrade adapter to preCICE version 3 API #34

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

Merged
merged 19 commits into from
Apr 23, 2025

Conversation

khalilhkiri
Copy link
Contributor

No description provided.

@BenjaminRodenberg BenjaminRodenberg self-requested a review April 22, 2025 06:35
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

The installation instructions for a UDF under https://ansyshelp.ansys.com/public/account/secured?returnurl=/Views/Secured/corp/v242/en/flu_udf/flu_udf_sec_compile_tui.html say: "Copy your source file (for example, myudf.c) to the source directory (src)." I think we should stay consistent with this approach and provide the sources under fluent-adapter/src and then ask the user to copy them to the example directory. Therefore, we should avoid the code duplication of examples/Cavity2D/libudf/src.

Additionally: Do you want to mention https://ansyshelp.ansys.com/public/account/secured?returnurl=/Views/Secured/corp/v242/en/flu_udf/flu_udf_sec_compile_tui.html in the README.md?

Comment on lines +13 to +16
a = 0.01 # Thickness [m]
MI = a**4 / 12 # Area moment of inertia
ME = 117e9 # Modulus of elasticity (117 GPa)
ll = 1.0 # Beam length [m]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
a = 0.01 # Thickness [m]
MI = a**4 / 12 # Area moment of inertia
ME = 117e9 # Modulus of elasticity (117 GPa)
ll = 1.0 # Beam length [m]
a = 0.01 # Thickness [m]
MI = a**4 / 12 # Area moment of inertia of square cross section
ME = 117e9 # Modulus of elasticity (117 GPa) of copper
ll = 1.0 # Beam length [m]

better keep this information. Otherwise these number are just "magic" numbers.


dim = interface.get_mesh_dimensions(mesh_name)
print("Mesh dimensions:", dim)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("Mesh dimensions:", dim)

let's remove this debug statement

print("coordinate array to be sent to set_mesh_vertices = {}".format(coords))
print("mesh_name sent to set_mesh_vertices = {}".format(mesh_name))
print("mesh_id sent to set_mesh_vertices = {}".format(mesh_name))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("mesh_id sent to set_mesh_vertices = {}".format(mesh_name))
print("mesh_name sent to set_mesh_vertices = {}".format(mesh_name))

no?

Comment on lines 77 to 63
forces = interface.read_data(mesh_name, force_name, vertexIDs, dt)
forces = interface.read_data("beam", "Forces", vertexIDs, dt)
Copy link
Member

Choose a reason for hiding this comment

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

Generally ok, but why? I usually prefer to define mesh_name somewhere at the top during initialization. At some point we will have to change "beam" to "Beam-Mesh". If we use a variable mesh_name = "beam" we only have to change this at one place and there is a lower risk to forget it. They way how it is currently written here is a bit more risky in this regard.

Comment on lines 83 to 69
interface.write_data(mesh_name, displ_name, vertexIDs, displacements)
interface.write_data("beam", "Displacements", vertexIDs, displacements)
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Comment on lines 63 to 66
if interface.requires_initial_data():
interface.write_data(mesh_name, force_name, vertexIDs, forces)
interface.write_data(mesh_name, displ_name, vertexIDs, displacements)

Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this? Generally we should initialize data. If this does not work at the moment: Let's at least put a todo here.

@@ -0,0 +1,5 @@
CSOURCES= fsi_udf.c fsi.c
HSOURCES= fsi.h
FLUENT_INC= /lrz/sys/applications/ansys/v241/fluent
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FLUENT_INC= /lrz/sys/applications/ansys/v241/fluent
FLUENT_INC= /path/to/fluent # TODO has to be provided by user

Is # the correct way to define a comment?

@@ -0,0 +1,5 @@
CSOURCES= fsi_udf.c fsi.c
HSOURCES= fsi.h
FLUENT_INC= /lrz/sys/applications/ansys/v241/fluent
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FLUENT_INC= /lrz/sys/applications/ansys/v241/fluent
FLUENT_INC= /path/to/fluent # TODO has to be provided by user

See above.

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 we should not duplicate the src files for the adapter here and just provide them under fluent-adapter/src. Be careful: Don't delete this file (and the others) but copy-paste them over the ones in fluent-adapter/src (I'm not sure if there are any changes you want to keep).

Comment on lines +61 to +62
<relative-convergence-measure limit="1e-6" data="Displacements" mesh="beam"/>
<relative-convergence-measure limit="1e-6" data="Forces" mesh="beam"/>
Copy link
Member

Choose a reason for hiding this comment

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

Why? This should not harm but I would like to know if this is necessary or not.

@BenjaminRodenberg BenjaminRodenberg changed the base branch from update-v3 to develop April 22, 2025 08:43
</participant>

<m2n:sockets from="Fluent" to="CSMdummy"/>
<m2n:sockets acceptor="Fluent" connector="CSMdummy" exchange-directory=".."/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<m2n:sockets acceptor="Fluent" connector="CSMdummy" exchange-directory=".."/>
<m2n:sockets acceptor="Fluent" connector="CSMdummy"/>

Does not harm but this is also not needed.

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg 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 to me. I did not test the adapter myself but it is confirmed to be working by @khalilhkiri.

@BenjaminRodenberg BenjaminRodenberg changed the title adapter version 3 upgrade Upgrade adapter to preCICE version 3 API Apr 23, 2025
@BenjaminRodenberg BenjaminRodenberg merged commit 636bc34 into precice:develop Apr 23, 2025
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.

2 participants