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/mesh motion #23

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Feature/mesh motion #23

wants to merge 18 commits into from

Conversation

tmaric
Copy link
Contributor

@tmaric tmaric commented May 1, 2024

Working MLP mesh deformation for OpenFOAM's wing2D_pimpleFoam tutorial.

@al-rigazzi
Copy link

@tmaric any reason why this has not been merged? I would have no objections to it.

@@ -3,6 +3,6 @@ cd "${0%/*}" || exit # Run from this directory
. ${WM_PROJECT_DIR:?}/bin/tools/RunFunctions # Tutorial run functions
#------------------------------------------------------------------------------

runApplication checkMesh -constant -writeAllFields
runApplication mpirun -np 4 checkMesh -constant -writeAllFields -parallel
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use runParallel?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tmaric I checked the notebook. I am not sure about the workaround using subprocess. The point of using SmartSim is that it manages resources and jobs. When using subprocess, we take resource management and exception handling into our own hands. Also, when running the notebook on a cluster, the preprocessing and cleanup would be executed on the login node. I'd prefer a solution employing the SmartSim API all the way through, which requires dealing with the subfolder issue. I am not yet sure how to deal with the new model-specific subfolders. One way would be to give all the models the same name and use overwrite=True. However, that workaround would create unnecessary copy operations, and I guess it does not work if an app modifies an existing dictionary (e.g., for setting an inflow profile). Maybe @al-rigazzi can give a hint on how to disable or work around the subfolder creation. Should we use the API at a lower level?

There is also a typo in the first print statement after running Allclean.

related to #29

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked the documentation of Experiment.create_model(), and the solution seems to be quite simple. One can use exp.create_model(..., path="path/to/simulation") to obtain the same behavior as before (not yet tested, though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The subprocess is not the workaround for the issue of copying config files into the model sub-directory introduced in smartsim 0.8.0.

The subprocess was used to separate pre-processing from the CFD+ML algorithm. The pre processing requires blocking mode, and CFD+ML non-blocking, this was the reason for introducing it originally. Additionally, more complex pre-processing may be necessary, which the user doesn't want to implement into a SmartSim workflow. For example, wingMotion requires simpleFoam to be run in a case before CFD+ML algorithm is started, and for that run there is no need to use smartredis at all.

That was the motivation, but I agree 100% that a consistent way would be to implement everything in a SmartSim workflow.

For that, I think we just need two experiments because of blocking/non blocking requirement: pre processing experiment and running experiment. We add meshing, initialization and decomposition as blocking steps in the preprocessing experiment. We run the CFD+ML experiment as is. This should be sufficient and consistent, without having to use any workarounds.

does this sound OK to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Creating multiple Experiments would be one solution, but it's also possible to use multiple different models within one Experiment (example). You can decide to use blocking/nonblocking for each model individually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then this is the best option IMO, I will try this first, alongside providing the folder argument to the model to avoid the 'to_copy' and 'generate' lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

path (Optional[str], default: None) – path to where the Model should be executed at runtime

This is not the same as of_model.attach_generator_files(to_copy="spinningDisk") copying files into the model sub-folder. FYI @AndreWeiner

https://www.craylabs.org/docs/api/smartsim_api.html#smartsim.experiment.Experiment.create_model

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.

4 participants