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

simplifying the code in the other source files under the refinement folder #1665

Merged
merged 1 commit into from May 15, 2017

Conversation

yinghe616
Copy link
Contributor

I didn't change any functionality or logic of those code. When I wrote the new mesh refinement composition_approximate_gradient.cc, just looked around other sources files in the same folder, and I found that some of those codes can also be simplified.


// Scale gradient in each cell with the correct power of h. Otherwise,
// error indicators do not reduce when refined if there is a density
// jump. We need at least order 1 for the error not to grow when
// refining, so anything >1 should work. (note that the gradient
// itself scales like 1/h, so multiplying it with any factor h^s, s>1
// will yield convergence of the error indicators to zero as h->0)
indicators[i] *= std::pow(cell->diameter(), power);
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same logic as before? It looks like we would scale indicators[i] several times here before. I think the new logic is correct and the old one is incorrect for more than one field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh..., only this one is different than others. :D It was mentioned as in issue #1659

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the old logic was incorrect, see #1659.

@tjhei
Copy link
Member

tjhei commented May 15, 2017

Looks good. /run-tests

@tjhei
Copy link
Member

tjhei commented May 15, 2017

okay, can you adjust the test output then?

@bangerth
Copy link
Contributor

We have one test that now produces different output, and it is exactly the one we suspected -- tests/refinement_composition_gradient. @yinghe616 -- can you apply the changes from here: http://www.math.clemson.edu/~heister/cib/aspect-8.5-gcc-hd/4516bba1be0e92cf16d357e24b9840b37c47602f/changes-gcc.diff to this patch?

Maybe as a general rule: if you fix one issue, and then do a bunch of other simplifications, it's often useful to do so in separate pull requests. This way the uncontroversial parts can easily be committed, and we can discuss the bug fixes separately from the rest.

@YHUCD YHUCD force-pushed the simplify_composition_refinement branch from 4516bba to ed77759 Compare May 15, 2017 01:43
@yinghe616
Copy link
Contributor Author

ok. I just recovered composition_gradient.cc and removed from this Patch. I'll open a different pull request with a new test example results. I was totally forgot that it could be possible to give a different results than before. :D

@tjhei tjhei merged commit 46a77da into geodynamics:master May 15, 2017
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

3 participants