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 eos information to floors, misc UserWorkBeforeOutput fix #210

Merged
merged 14 commits into from
Mar 20, 2024

Conversation

AstroBarker
Copy link
Collaborator

@AstroBarker AstroBarker commented Mar 19, 2024

This PR adds knowledge of eos bounds to the floors. Specifically, for tabulated EOS, demand that the floors do not extrapolate off of the table. This required making the bounds param mutable.

Currently, for ideal gas, the option to enforce user set bounds is supported by supplying, e.g., <eos>/rho_min in the input deck. If unspecified, reasonable defaults are set.

Also fixed a misc bug in the UserWorkBeforeOutput code where a package and param were pulled out inside a device kernel that caused a crash on device.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

I think i see why you're getting segfaults. See comments below. After making those changese everywhere relevant (I only highlighted once), give it a try on device again.

src/fixup/fixup.cpp Outdated Show resolved Hide resolved
src/fixup/fixup.cpp Outdated Show resolved Hide resolved
src/fixup/fixup.cpp Show resolved Hide resolved
src/fixup/fixup.hpp Show resolved Hide resolved
src/fixup/fixup.hpp Outdated Show resolved Hide resolved
src/fixup/fixup.hpp Outdated Show resolved Hide resolved
Comment on lines 327 to 331
auto MaxDensity = [](MeshData<Real> *md) {
return ReduceOneVar<Kokkos::Max<Real>>(md, fluid_prim::density::name(), 0);
};

hst_vars.emplace_back(HistoryOutputVar(HstMax, MaxDensity, "maximum density"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this branch might be out of date with main... double check you and @mari2895 are in sync so your PRs don't undo each other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this is the same as current main, unless I missed something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a diff showing here then? Odd.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see it now... not sure why it's different from main, don't think I did this myself..

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe just merge main in again... see if that fixes it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AstroBarker Looks the same on my branch too. Don't see anything different.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but why is it deleted? should not be deleted

@Yurlungur
Copy link
Collaborator

Ready for re-review, @AstroBarker ?

@AstroBarker AstroBarker changed the title WIP: Add eos information to floors and ceilings Add eos information to floors, misc UserWorkBeforeOutput fix Mar 20, 2024
@AstroBarker
Copy link
Collaborator Author

Ready for re-review, @AstroBarker ?

Good to go

@AstroBarker AstroBarker added bug Something isn't working enhancement New feature or request labels Mar 20, 2024
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Two tiny nitpicks. Otherwise this is good to go.

src/fixup/fixup.hpp Outdated Show resolved Hide resolved
src/fixup/fixup.hpp Outdated Show resolved Hide resolved
Comment on lines 327 to 331
auto MaxDensity = [](MeshData<Real> *md) {
return ReduceOneVar<Kokkos::Max<Real>>(md, fluid_prim::density::name(), 0);
};

hst_vars.emplace_back(HistoryOutputVar(HstMax, MaxDensity, "maximum density"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a diff showing here then? Odd.

@Yurlungur Yurlungur merged commit 00275ce into main Mar 20, 2024
2 checks passed
@Yurlungur Yurlungur deleted the eos_floors branch March 20, 2024 22:56
Copy link
Collaborator

@mari2895 mari2895 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for explaining!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants