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

Fix the units for the darcy velocity postprocessor #6129

Merged

Conversation

danieldouglas92
Copy link
Contributor

@danieldouglas92 danieldouglas92 commented Nov 4, 2024

Within the darcy velocity postprocessor, the multiplier which converts the units of the darcy velocity to years instead of seconds was not being multiplied by the solid velocity, this PR fixes this unit discrepancy.

@danieldouglas92 danieldouglas92 force-pushed the fix_units_in_darcy_velocity_viz branch from ac71f79 to 7c5f19c Compare November 4, 2024 18:43
@danieldouglas92
Copy link
Contributor Author

I'm not sure why the dealii-master tests suite is failing, but this seems like something outside of the scope of this PR?

@gassmoeller
Copy link
Member

/rebuild

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.

Nice find. The failing test for deal.II master was unrelated and should be fixed, I restarted the tester to double check.

However, since none of our own tests are failing that means we currently have no test for this postprocessor. Would it be hard to add one either in this PR or as a follow-up?

@danieldouglas92
Copy link
Contributor Author

danieldouglas92 commented Nov 5, 2024

There is actually a test for this postprocessor in darcy_convection_step.prm, but the reason this bug wasn't found in that test is that the solid velocity was 0! I could modify another test which uses the darcy field advection method with a non zero solid velocity though?

@tjhei
Copy link
Member

tjhei commented Nov 5, 2024

I could modify another test

that would be great!

@danieldouglas92 danieldouglas92 force-pushed the fix_units_in_darcy_velocity_viz branch from 7c5f19c to 5f12048 Compare November 7, 2024 16:48
@danieldouglas92
Copy link
Contributor Author

I decided it would be best to just update the existing test and make the solid velocity non-zero, there wasn't an existing test where it would be clear to see that the solid velocity was properly being taken into account in the post-processing.

@danieldouglas92 danieldouglas92 force-pushed the fix_units_in_darcy_velocity_viz branch from 5f12048 to 7fa2dc0 Compare November 7, 2024 17:59
@gassmoeller
Copy link
Member

/rebuild

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.

Most often it is best to add new tests instead of modifying existing ones, because we want tests to be specific to one feature. That being said, in this case the existing test was to test exactly this and just wasnt working as expected. So we can go ahead with this PR. One of the tester machines had trouble (likely unrelated to your PR). I restarted the testers, if they pass this is ready to merge. 👍

@gassmoeller gassmoeller merged commit 0b4a5c0 into geodynamics:main Nov 8, 2024
8 checks passed
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.

3 participants