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

Jint surfing #88

Merged
merged 15 commits into from
Sep 20, 2021
Merged

Jint surfing #88

merged 15 commits into from
Sep 20, 2021

Conversation

BoZeng1997
Copy link
Contributor

@BoZeng1997 BoZeng1997 commented Jul 19, 2021

A negative J2 invariant should never happen again. But I have not yet found out why the J integral over the boundary does not approach the value of the prescribed Gc.
In the folder /benchmark/AT2surfing, there are input deck for surfing boundary problem under AT-2 without irreversibility. As a standard phase-field fracture problem it still cannot return a close value to Gc.

Copy link
Owner

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

Please remove the scripts for submitting jobs. Also remove output files like surf_AT2_Gc2L0.35del1.16_dt1e-2_J5b_14523984.

benchmarks/AT2surfing/submit.sh Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
tutorials/mode1_brittle_fracture/submit.sh Outdated Show resolved Hide resolved
tutorials/mode1_cohesive_fracture/submit.sh Outdated Show resolved Hide resolved
@hugary1995
Copy link
Owner

hugary1995 commented Jul 19, 2021

Directly writing to standard streams (like std::cout) is prohibited. You can use them in your local branch for debugging purposes, but remember to remove those in a PR.

Copy link
Owner

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

Overall looks good. I suggest we start from cleaning up some unused comments. Then we can double check the implementation.

benchmarks/AT2surfing/fracture.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/fracture.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/fracture.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/fracture.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/fracture.i Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
@BoZeng1997 BoZeng1997 closed this Jul 19, 2021
@BoZeng1997 BoZeng1997 reopened this Jul 19, 2021
@BoZeng1997
Copy link
Contributor Author

I have cleaned unused comments and complete the necessary comments. But I did not make changes that will cause further changing in input deck. Were they changed, all of the input deck must be changed. I cannot guarantee a same result.

The problem right now is not with kumar's model, but j integral in general.

In the folder /benchmark/AT2surfing, there is input deck for surfing boundary problem under AT-2 without irreversibility (with irreversibility commented). The problem right now is that in the AT-2 model, with smaller mesh size, it still cannot return a close value to Gc.

@BoZeng1997
Copy link
Contributor Author

... the number of changes made is still very large. I think it is because i rm your examples. I thought that is a way not to track the change in it.

@hugary1995
Copy link
Owner

The number of lines of addition is huge because you've checked in many exodus files -- remove those.
The number of lines of deletion is huge because you've removed everything in the benchmarks folder. We do plan to remove the entire benchmarks folder at some point in the future, but don't do that for now.

@BoZeng1997
Copy link
Contributor Author

recovered benchmark examples.
cleared other redundancy.

@hugary1995
Copy link
Owner

Fantastic, thanks.

Copy link
Owner

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

We are getting close. I think I found the cause of your J-R curve issue.

Also, please enforce clang-format -- most long equations are pretty unreadable right now. If you need help setting up clang-format just let me know.

benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/fracture.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
@hugary1995
Copy link
Owner

hugary1995 commented Jul 19, 2021

Here is a decision for you to make: if you only want to demonstrate the usage of J-integral in this PR, then all you need are the source code for the J-integral postprocessor and one tutorial input file. If you also want to check in the Kumar model, then in addition to the source code that you have checked in, I also need to see at least one regression test/tutorial that demonstrates its usage.

I'm fine with both, it's up to you.

@hugary1995
Copy link
Owner

hugary1995 commented Jul 19, 2021

You can see why the tests are failing if you click on details. For example, the Precheck is currently failing because:

##########################################################################
ERROR: Your patch does not contain a valid ticket reference! (i.e. #1234)
Merge branch 'JintSurfing' of https://github.com/BoZeng1997/raccoon into test
clear
recover examples
Jintsurfing-clean2
AT2surfing-clean
for AT2 Jintegral Surfing only
GeneralizedKumar_issue_Jint
all kinds of kumar for test
lastindirectkumar
modifyGeneralizedExternalDrivingForce to have ce in elasticity.i
recover tutorials from upstream
recover tutorials
20210623_updateKumar_ProblemNAN
first_extdriving

ERROR: The following files contain banned keywords (std::cout, std::cerr, sleep, print_trace):
	src/materials/GeneralizedExternalDrivingForce.C
##########################################################################

@BoZeng1997
Copy link
Contributor Author

COPY FROM MOOSE WEBPAGE:

Referencing Issues
Every modification to MOOSE must reference an issue number. This means that every Pull Request (PR) that flows into MOOSE must have contain at least one commit that references an issue relevant to what you are working on (e.g. refs # (where is an issue number on the MOOSE GitHub issue page, such as #1234). If your PR completely addresses an issue, you can automatically close it by prepending "closes" or "fixes" to the issue reference (e.g., closes #1234). Issue numbers are automatically checked by our testing system.

@hugary1995
Copy link
Owner

If you don't find a relevant issue, you can create your own. Then you can modify your commit message to reference that issue.

@BoZeng1997
Copy link
Contributor Author

Here is a decision for you to make: if you only want to demonstrate the usage of J-integral in this PR, then all you need are the source code for the J-integral postprocessor and one tutorial input file. If you also want to check in the Kumar model, then in addition to the source code that you have checked in, I also need to see at least one regression test/tutorial that demonstrates its usage.

I'm fine with both, it's up to you.

For example, is the input deck already committed in this folder enough to be a tutorial input file for simply J integral?

I do also want to check the kumar model, since I have already committed the object and it cannot return a Jintegral value close to that of AT2. I want to know what kind of test is good enough as a regression test. can surfing boundary value problem serve as a regression test? we know the expected speed of the crack propagation but the result need to be obtained from paraview.

@hugary1995
Copy link
Owner

For example, is the input deck already committed in this folder enough to be a tutorial input file for simply J integral?

Yes.

I do also want to check the kumar model, since I have already committed the object and it cannot return a Jintegral value close to that of AT2. I want to know what kind of test is good enough as a regression test. can surfing boundary value problem serve as a regression test? we know the expected speed of the crack propagation but the result need to be obtained from paraview.

You might have misunderstood the meaning of regression test. Regression test is used to ensure consistency, not for correctness.

Take a look at other files in the tutorials folder, each tutorial has its own tests specification. Most of them are simply diffing the output against some gold files you check in. This is to make sure future changes to raccoon do not break you stuff.

@BoZeng1997
Copy link
Contributor Author

For example, is the input deck already committed in this folder enough to be a tutorial input file for simply J integral?

Yes.

I do also want to check the kumar model, since I have already committed the object and it cannot return a Jintegral value close to that of AT2. I want to know what kind of test is good enough as a regression test. can surfing boundary value problem serve as a regression test? we know the expected speed of the crack propagation but the result need to be obtained from paraview.

You might have misunderstood the meaning of regression test. Regression test is used to ensure consistency, not for correctness.

Take a look at other files in the tutorials folder, each tutorial has its own tests specification. Most of them are simply diffing the output against some gold files you check in. This is to make sure future changes to raccoon do not break you stuff.

I did misunderstand the regression test.

But for now, since we are not updating our moose, is one tutorial file, say kumar's model with surfing BVP, enough for kumar's usage?

@hugary1995
Copy link
Owner

Yes, that's sufficient. It will demonstrate the usage of both the J integral post processor and the nucleation model.

@hugary1995
Copy link
Owner

Just ping me when you think this PR is ready for review.

Copy link
Owner

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

Caught a few more minor things.

benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
benchmarks/AT2surfing/elasticity.i Outdated Show resolved Hide resolved
@BoZeng1997
Copy link
Contributor Author

It is ready for review now. I have made modifications to most of the issues mentioned above. I still keep the J2 and F_surface in the object GeneralizedExternalForce because it will be useful for debuging. They will not be stored in the future. The surfing boundary condition did not change.

@BoZeng1997
Copy link
Contributor Author

I am not sure it will pass the checks. I need to run for the bus now

@hugary1995
Copy link
Owner

Thanks, this is getting really close now. The "Travis CI" test fails because of documentation failure -- some classes are missing documentations.

Let's discuss about the boundary condition in tomorrow's group meeting.

A few minor things:

  • Now that you've added that benchmarks/surfingBVP example, the benchmarks/AT2surfing example becomes redundant, so lets remove that one.
  • We shouldn't be adding any new examples into that benchmarks folder -- those are our old examples. We want to migrate everything to the new tutorials folder. So could you move your benchmakrs/surfingBVP example into the tutorials folder? I think that will be a very useful tutorial for everyone. Also, let's try to keep it consistent with the naming convention in that folder, so instead of surfingBVP, let's rename it to something like surfing_BVP with an underscore.
  • One last thing you need to do is to tell the testing system how this new tutorial should be tested. To do that, you need to add a file called tests in the folder of your example. In that tests file, you should describe how the test should run. You can simply copy from another tests from an existing tutorial. Make sure you only run it for one or two steps so that the test can finish within 20 seconds.
  • One other thing you could do, although not necessary, is to document the tutorial. To document a tutorial, first add an entry to the menu in doc/menu.yml, something like 12. Steady crack propagation: tutorials/12_surfing.md. Then add the actual documentation page at doc/content/tutorials/12_surfing.md. You can copy from existing documentations, e.g. 01_small_deformation_elasticity.md, as a starting point.

As a reminder, to build documentation on your own machine:

  • commit everything, make sure everything is tracked by git
  • compile your application, make sure it is up-to-date
  • cd doc
  • ./moosedocs.py build --serve, this will build the documentation and serve the website at your localhost.

@hugary1995
Copy link
Owner

Hey Bo, have you had a chance to address those comments? If you need help with anything just let me know.

@BoZeng1997
Copy link
Contributor Author

I was not on it this week. My Mac machine became slow again and I have to focus on one task. It is slow even just typing on Github page. My laptop is back and I am preparing it back to work. I will be back on this soon.

@hugary1995
Copy link
Owner

Okay no worries -- wasn't pushing you.

@BoZeng1997
Copy link
Contributor Author

Do you accept a kumar's that might be wrongly implemented? I am still looking for the reason I cannot have Jintegral = Gc.

@hugary1995
Copy link
Owner

It's not related to the nucleation model, is it? You've shown that J isn't exactly equal to Gc even with standard AT1 or AT2.

@hugary1995
Copy link
Owner

But I do remember that when I tried the surfing problem, I got J = Gc for a purely elastic case. Let me play with it and see if I can reproduce what I had before.

@BoZeng1997
Copy link
Contributor Author

I see why the Test test is failing for you. That's because you committed a new submodule moose. Please don't do that.

How do I remove this redundant moose safely?

@BoZeng1997
Copy link
Contributor Author

Yes, you should worry about those errors. These errors tell you exactly what files are missing. These errors will prevent the Documentation test from passing, and a PR cannot be merged with a failing test.

The ./moosedocs.py build --serve process almost stuck at [I 210827 14:41:40 handlers:64] Start detecting changes . Is this normal?

It is not stuck, it is detecting changes. You should be able to view the documentation website locally at your localhost.

How long should this step usually take?

@BoZeng1997
Copy link
Contributor Author

I install the most up-to-date clang format 11 on my Mac machine and tried clang-format the src code of GeneralizedExternalDrivingForce.C. The result is the same as the one which got rejected by this clang-format check in the first place. So it was not the problem of my clang-format version. There has to be some setting in clang format that should not be default value. I also tried format by hand and it solved most of the problem.

It is ready for review.

Copy link
Owner

@hugary1995 hugary1995 left a comment

Choose a reason for hiding this comment

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

Some minor naming questions and code suggestions.

include/auxkernels/ConditionalBoundsAux.h Outdated Show resolved Hide resolved
include/auxkernels/ConditionalBoundsAux.h Outdated Show resolved Hide resolved
src/auxkernels/ConditionalBoundsAux.C Outdated Show resolved Hide resolved
src/auxkernels/ConditionalBoundsAux.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
src/materials/GeneralizedExternalDrivingForce.C Outdated Show resolved Hide resolved
@hugary1995
Copy link
Owner

I just pushed three commits cleaning up everything. Take a look at the changes I made -- if you are fine with those then this PR is good to go.

I was mostly making minor style changes. But one mistake I noticed is that you named the test specification test instead of tests, and as a result the regression test wasn't being run. I've fixed that for you.

@hugary1995
Copy link
Owner

I would also like to thank you for your contribution! It is hard for everyone to make their first pull request. Hopefully you've learnt something from this PR. You should definitely be proud of this work!

hugary1995
hugary1995 previously approved these changes Sep 20, 2021
@hugary1995
Copy link
Owner

@milljm This is the PR waiting for CIVET tests.

@BoZeng1997
Copy link
Contributor Author

Yeah, I think those modifications are great. Thank you for being so patient with this PR! I did learned a lot!

@hugary1995 hugary1995 merged commit eb31a91 into hugary1995:master Sep 20, 2021
@hugary1995 hugary1995 mentioned this pull request Sep 20, 2021
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.

None yet

2 participants