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

Add STL geom_type for EB in Source/InitEB.cpp #771

Merged
merged 15 commits into from
Mar 27, 2024

Conversation

ejyoo921
Copy link
Contributor

The current version of PeleC does not recognize "stl" as an eb.geom_type.
=> This is edited in Source/InitEB.cpp (line 355).

Additionally, we exclude stl geom_type from the amrex::EB2:: addFineLevels(max_level - eb_max_level) condition: => This is not updated or implemented well in AMReX-EB code (yet). => See Source/InitEB.cpp (line 370)

Please find "EY" comments for these updates.

As a test case, I used Exec/RegTests/EB-C9:
An example stl file => cylinder.stl
and an input file to use this stl => eb-c90-cylinder.inp
are included. Please see the last few lines in the input file for the EB-STL parameters.

Please contact me, Eunji, if you have any questions.

=> this is edited in Source/InitEB.cpp (line 355).
Additionally, we exclude stl geom_type from the amrex::EB2:: addFineLevels(max_level - eb_max_level) condition:
=> This is not updated or implemented well in AMReX-EB code (yet).
=> See Source/InitEB.cpp (line 370)

Please find "EY" comments for these update.
Copy link
Contributor

@baperry2 baperry2 left a comment

Choose a reason for hiding this comment

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

Couple minor things on the implementation. I'll have some suggestions on the example case later.

Source/InitEB.cpp Outdated Show resolved Hide resolved
Source/InitEB.cpp Show resolved Hide resolved
Build/cmake.sh Outdated Show resolved Hide resolved
@ejyoo921
Copy link
Contributor Author

profiles.pdf

I ran the example case with the cylinder-x-dir.STL file, which is aligned in the x-direction. I manually scaled the cylinder in the input file (eb-c9-cyhliner.inp) by adding "eb2.stl_scale = 50" line. The original cylinder has a radius of 0.5.

@baperry2
Copy link
Contributor

I think we should update the regtest to use the STL version instead of the version with the cylinder geom_type. We have plenty of other tests that use the cylinder geometry and this would serve as both a test and a demo of the STL capability.

@ejyoo921 to do this, just modify eb-c9.inp to look like your eb-c9-cylinder.inp, and make sure you commit the new version of the STL file as well. You can then delete the old STL and eb-c9-cylinder.inp.

@marchdf
Copy link
Contributor

marchdf commented Mar 26, 2024

A quick check (that maybe you already did) is if those 2 cases give the same answer.

@ejyoo921
Copy link
Contributor Author

A quick check (that maybe you already did) is if those 2 cases give the same answer.

The running study case for density and its error?

…to give a larger one - It seems like using the eb2.stl_scale parameter is somehow not okay. We may investigate further about this. For now, use the new file 'cylinder-x-r50.stl' for the C9 test.

We also modified the eb-c9.inp to show how to use stl file for eb2. Former way of eb2.geom_type = cylinder is commented at the end
Copy link
Contributor

Choose a reason for hiding this comment

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

This file can be deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

This file can also be deleted

@baperry2
Copy link
Contributor

The running study case for density and its error?

Yeah if you can re-run the case study with your latest changes that should be sufficient.

It would also be interesting to run amrex fcompare to compare both the initial and final plot files to those for the old cylinder. I'd imagine there will be small diffs in the vfrac field that lead to small diffs in other fields over time.

@ejyoo921
Copy link
Contributor Author

The running study case for density and its error?

Yeah if you can re-run the case study with your latest changes that should be sufficient.

It would also be interesting to run amrex fcompare to compare both the initial and final plot files to those for the old cylinder. I'd imagine there will be small diffs in the vfrac field that lead to small diffs in other fields over time.

yup I am running it now. I see that AMReX could not load the new stl file I just pushed 🤦🏻‍♀️

@ejyoo921
Copy link
Contributor Author

profiles.pdf

Density, entropy, and density error plots

@ejyoo921
Copy link
Contributor Author

Where can I see the test file or script?
25/36 Test #25: eb-c9 ............................***Failed 0.14 sec

@ejyoo921
Copy link
Contributor Author

I was checking the Tests/CMakeLists.txt file, and my guess is that it copies the input file - {TEST_NAME}.inp - to the test working directory but does not bring the stl file. I had the same issue when I was testing on my machine.

Copy files to test working directory

file(COPY ${CURRENT_TEST_SOURCE_DIR}/${TEST_NAME}.inp DESTINATION "${CURRENT_TEST_BINARY_DIR}/")
file(COPY ${TEST_FILES} DESTINATION "${CURRENT_TEST_BINARY_DIR}/")

@marchdf
Copy link
Contributor

marchdf commented Mar 27, 2024

ah yes it needs to bring over the stl file as well. You can edit this line: file(GLOB TEST_FILES "${CURRENT_TEST_SOURCE_DIR}/*.dat" "${CURRENT_TEST_SOURCE_DIR}/*.py") in setup_test and add *.stl to that list.

@@ -365,8 +366,12 @@ initialize_EB2(

// Add finer level, might be inconsistent with the coarser level created
// above.
if (geom_type != "chkfile") {
// EY: This condition is not acceptable in AMReX with stl format
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a silent comment, maybe we could replace this with an actual Abort? That way we know it breaks loudly and we can always go back and check if amrex accepts it later? Or maybe they never will? Or maybe I have no idea what I am talking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

The invalid condition gets caught by the AMREX_ALWAYS_ASSERT below. The addFineLevels function isn't supported for chkfile and stl, but it's ok to just skip if no levels are being added. So we catch the error for either of these options only when max_level != eb_max_level

2) Changed the cylinder-r50.stl location in the eb-c9.inp file by adjusting the center
3) Edited the setup_test in the Tests/CMakeLists.txt:
	=>  Added *.stl to copy for tests (line 53)
@ejyoo921
Copy link
Contributor Author

The latest (Hopefully the last one) results with corrected cylinder location.

profiles.pdf

@baperry2 baperry2 enabled auto-merge (squash) March 27, 2024 19:46
@baperry2 baperry2 merged commit 237b9c8 into AMReX-Combustion:development Mar 27, 2024
14 checks passed
WeiqunZhang pushed a commit to AMReX-Codes/amrex that referenced this pull request Sep 2, 2024
…#4123)

## Summary

The `addFineLevels` function is not supported for EB2 for chk_file and
stl geometries. However, it may be called in some for some trivial cases
where it is adding 0 levels, in which case it is a no-op. There is no
reason to abort in those cases.

## Additional background

For PeleC, a work-around was put in to not call the function in the
trivial cases (AMReX-Combustion/PeleC#771). I
was thinking about adding the same work around to address the same thing
in PeleLMeX (AMReX-Combustion/PeleLMeX#407),
but maybe it would be better to simply allow the function to be called
in trivial cases. If there's a reason not to do this, I'll just put the
workaround in for PeleLMeX.
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