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.

Sign up for GitHub

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

Jump to bottom

allow continuous and discontinuous compositions #5909

Merged
merged 2 commits into from
Jun 18, 2024
Merged

allow continuous and discontinuous compositions #5909

merged 2 commits into from
Jun 18, 2024

Conversation

tjhei
Copy link
Member

@tjhei tjhei commented Jun 14, 2024

part of #5748

@tjhei tjhei force-pushed the composition-really-some-disc branch from b672ac1 to 23ac19c Compare June 14, 2024 19:57
@tjhei
Copy link
Member Author

tjhei commented Jun 14, 2024

CG and DG advection field (see test):
image

@tjhei tjhei force-pushed the composition-really-some-disc branch from 23ac19c to d386856 Compare June 14, 2024 20:00
gassmoeller
gassmoeller reviewed Jun 15, 2024
View reviewed changes
Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

The structure looks good and except for your test you didnt break anything else ;-). I am sure we will find some problem eventually. Do you want to include the particle support in this PR or disallow particles with mixed CG/DG fields for now?

@tjhei tjhei force-pushed the composition-really-some-disc branch from d386856 to 74e06f6 Compare June 16, 2024 18:50
@tjhei
Copy link
Member Author

tjhei commented Jun 16, 2024

Do you want to include the particle support in this PR or disallow particles with mixed CG/DG fields for now?

It is now no longer allowed. I think implementation is a bit more involved than expected.

@tjhei tjhei force-pushed the composition-really-some-disc branch from 59e12bb to 2e60d0a Compare June 16, 2024 18:59
@tjhei
Copy link
Member Author

tjhei commented Jun 16, 2024

updated.

@tjhei tjhei force-pushed the composition-really-some-disc branch from 2e60d0a to 3c975ef Compare June 16, 2024 19:06
@tjhei
Copy link
Member Author

tjhei commented Jun 18, 2024

This might be good to go!

gassmoeller
gassmoeller reviewed Jun 18, 2024
View reviewed changes
Copy link
Member

@gassmoeller gassmoeller 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 except for my minor comment, can be merged with those modifications. Is this the time to write a changelog entry?

source/simulator/introspection.cc
}

{
const auto variables = fevs.variables_with_name("compositions");
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
const auto variables = fevs.variables_with_name("compositions");
const auto composition_variables = fevs.variables_with_name("compositions");

tests/composition_cg_dg_fem.prm Outdated
@@ -0,0 +1,95 @@
# A test to check discontinuous composition discretization.
Copy link
Member

Choose a reason for hiding this comment

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

Description needs an update

Copy link
Contributor

Choose a reason for hiding this comment

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

If you copied the test from some other test, can you include that test and only list the lines that are different?

Copy link
Member Author

Choose a reason for hiding this comment

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

is this really what we do? wouldn't that create annoying dependencies between now unrelated tests?

jdannberg
jdannberg approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@jdannberg jdannberg 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 can't follow all the details, but what I understood made sense to me.)
Thank you for all your work on this, I think it's a really cool new feature!

tests/composition_cg_dg_fem.prm Outdated
@@ -0,0 +1,95 @@
# A test to check discontinuous composition discretization.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you copied the test from some other test, can you include that test and only list the lines that are different?

@tjhei tjhei force-pushed the composition-really-some-disc branch from 8a98032 to 33630e3 Compare June 18, 2024 15:56
@tjhei
Copy link
Member Author

tjhei commented Jun 18, 2024 •
edited
Loading

updated (except including the other test). changelog added.

gassmoeller
gassmoeller approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

I am ok merging it with the copied test. I think whether to include or to copy a test depends on the specific case. We have some tests that have 8 variations with just a few lines changed between them, there I think it makes more sense to include a base file. For a single new test with some changes it is not as important.

@gassmoeller gassmoeller merged commit 7b8dde8 into geodynamics:main Jun 18, 2024
8 checks passed
@tjhei tjhei deleted the composition-really-some-disc branch June 19, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdannberg jdannberg jdannberg approved these changes

@gassmoeller gassmoeller gassmoeller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tjhei @jdannberg @gassmoeller