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

More GeometryInfo work, the questionable part. #4430

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

bangerth
Copy link
Contributor

@bangerth bangerth commented Dec 9, 2021

This is the part of #4425 that didn't work yesterday. I checked that at least on my system, there is no difference between the before/after outputs of the merged_chunks_2D test that failed yesterday, though the output is (expectedly) slightly different than what's stored.

/rebuild

@bangerth
Copy link
Contributor Author

bangerth commented Dec 9, 2021

/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.

Maybe the tester was a bit overzealous when comparing the numbers in the other PR. I remember that the difference was something like 1e-8 and I thought our numdiff tolerance was coarser anyway. But could you replace the 1 << x line with what I suggested earlier: (dim == 3) ? 4 : 2;? I think it is just easier to understand.

@bangerth
Copy link
Contributor Author

Bizarre that it works now. I've made the change you requested (just the other way around :-) )

const auto min_distance_to_first_vertex =
std::min_element(distances_to_first_vertex.begin(),
distances_to_first_vertex.end());
distances_to_first_vertex.begin()+face->n_vertices());
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think I see what is happening, another random test failure due to access to non-initialized memory. Does this need to be the following? (see the size of the array above)

Suggested change
distances_to_first_vertex.begin()+face->n_vertices());
distances_to_first_vertex.begin()+face->n_vertices()-1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're totally right! How insidious! How did this work before?

Copy link
Contributor Author

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

OK, let's try this again!

const auto min_distance_to_first_vertex =
std::min_element(distances_to_first_vertex.begin(),
distances_to_first_vertex.end());
distances_to_first_vertex.begin()+face->n_vertices());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're totally right! How insidious! How did this work before?

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, feel free to merge if the tests pass.

Before the call was to distances_to_first_vertex.end() which worked correctly.

@bangerth bangerth merged commit bb10bb2 into geodynamics:master Dec 10, 2021
@bangerth bangerth deleted the GI4 branch December 10, 2021 21:10
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.

2 participants