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 tests #3275

Merged
Merged

Conversation

@naliboff
Copy link
Contributor

naliboff commented Oct 29, 2019

This PR adds two new tests for the Drucker Prager derivatives, which follow the structure of the visco plastic derivatives tests (e.g, visco_plastic_derivatives_2d and visco_plastic_derivatives_3d). These changes are needed, as #3257 requires simulator access during the tests.

For new features/models or changes of existing features:

  • I have tested my new feature locally to ensure it is correct.
  • I have created a testcase for the new feature/benchmark in the tests/ directory.
  • I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.
@naliboff

This comment has been minimized.

Copy link
Contributor Author

naliboff commented Oct 29, 2019

@MFraters - can you take a look at this when you have a chance? If the new tests look reasonable, I'll do a follow-up commit removing the original test.

Copy link
Contributor

MFraters left a comment

Hey John,

I checked the derivatives against the derivatives in the old test, and they are the same. So feel free to add a commit removing the old test.

@naliboff

This comment has been minimized.

Copy link
Contributor Author

naliboff commented Oct 29, 2019

@MFraters - thanks! Let me know if you would like these two commits merged.

@gassmoeller - Is the practice now to add change log entries for these types of minor test updates?

@gassmoeller

This comment has been minimized.

Copy link
Contributor

gassmoeller commented Oct 29, 2019

No, we only need changelog entries for:

  • new features
  • bugfixes
  • changes to existing features that affect users

So no need to document changed tests.

Feel free to merge once the testers are happy.

Copy link
Contributor

MFraters left a comment

I just noticed a small thing. Could you address that, or explain why you have chosen that wording?

{
std::cout << std::endl << "Test with dimension " << dim << std::endl;

std::cout << std::endl << "Testing DruckerPrager derivatives against analytical derivatives " << std::endl;

This comment has been minimized.

Copy link
@MFraters

MFraters Oct 29, 2019

Contributor

could you check the above sentence? Shouldn't it be: Testing DruckerPrager derivatives against finite difference derivatives? Same for the other test.

This comment has been minimized.

Copy link
@naliboff

naliboff Oct 29, 2019

Author Contributor

@MFraters - yes, the wording you used above is correct. Thanks for catching that!

@naliboff

This comment has been minimized.

Copy link
Contributor Author

naliboff commented Oct 29, 2019

@MFraters - comment addressed, I think should be ready to merge unless you would like to have another look.

@MFraters MFraters merged commit e7f7a02 into geodynamics:master Oct 30, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins.tjhei.info/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.