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.

Sign up for GitHub

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

Jump to bottom

Feature fix quality metrics #101

Merged
merged 22 commits into from
Mar 10, 2017
Merged

Feature fix quality metrics #101

merged 22 commits into from
Mar 10, 2017

Conversation

roigcarlo
Copy link
Member

This should fix #94 and fix #85.

As a resume this PR:

  • Corrects the problem with the VolumeToAverageEdgeLength quality function for tetrahedras
  • Fixed a bug that was causing an error if tests were compiled with clang
  • Completed the missing tests
  • Extended the tests for tetrahedras so a non-unitary geometry is also tested
  • Non implemented functions (regualrity and surface area) are now check that the excpetion is trown rather than the correct value.
  • Fixed KRATOS_CHECK_EXCEPTION while being called multiple times inside the same test

@loumalouomega
Copy link
Member

NICE WORK

pooyan-dadvand
pooyan-dadvand reviewed Feb 28, 2017
View reviewed changes
Copy link
Member

@pooyan-dadvand pooyan-dadvand left a comment

Choose a reason for hiding this comment

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

I think there are some compatibility issues for windows to be checked

kratos/testing/tester.cpp Outdated
@@ -228,8 +228,7 @@ namespace Kratos
{
// creating the regex pattern replacing * with ".*"

#if defined(__GNUC__) &&( __GNUC__ < 4 || \
(__GNUC__ == 4 && (__GNUC_MINOR__ < 9)))
#if __cplusplus <= 199711L
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 also visual studio compatible?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was a problem in VS 2012 with this but since we already use 2015 it should be fine. I will test it again to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like they still define this as 199711L.

The question is, since we now have C+11 as a requisite can't we just compile the regex code as is?

Copy link
Member

Choose a reason for hiding this comment

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

I think that this particular feature (regex) was still broken on relatively recent gccs, anyway Pooyan will remember more about this

Copy link
Member

Choose a reason for hiding this comment

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

Exactly as @RiccardoRossi saying, gcc 4.8 which is one of the popular old one does not support it and for me this function wasn't enough for drop it

kratos/tests/geometries/test_geometry.h Outdated
@@ -28,14 +28,15 @@
namespace Kratos {
namespace Testing {

static std::ofstream out("/dev/null");
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 windows compatible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not.
As we spoke, maybe I should just remove that ?

Copy link
Member Author

@roigcarlo roigcarlo Mar 1, 2017 •
edited

Choose a reason for hiding this comment

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

At the end it was working because std::cout.rdbuf(out.rdbuf()); was evaluated as NULL, which is an accepted buffer which has the same effect as /dev/null in linux. As it happens to be what we want im just making std::cout.rdbuf(nullptr);

According to this it also sets the std::ios::badbit which should prevent conversions in the output in case we eventually have any.

Copy link
Member

Choose a reason for hiding this comment

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

Seems ok, but in long term I would like to know why the same mechanism is not working in the tester

This was referenced Mar 1, 2017
Closed
Closed
@roigcarlo
Copy link
Member Author

@pooyan-dadvand I've made some changes since the last time:

  • The tester macro should now test the clang case apart
  • Fixed the length returning -nan from Length gives nan for inverted elements which leads to wrong quality measure #126. Notice that it uses the norm of the volume so it will be always positive
  • I've corrected a problem in the determinants while calculating the circumsphere
  • I added an inverted tetrahedra to the tests and extended the tests so it is checked as well, returning negative values in those who use volume as a metric (which was already working btw).

However, none of the quality functions were using the Length() method from the geometry, so the problems in #126 may still persist...

@roigcarlo roigcarlo added this to C++ in Kratos Testing Mar 8, 2017
pooyan-dadvand
pooyan-dadvand requested changes Mar 9, 2017
View reviewed changes
Copy link
Member

@pooyan-dadvand pooyan-dadvand left a comment

Choose a reason for hiding this comment

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

I would not let change the Volume method unless you have a very strong reason because it will break a lot of code.

kratos/geometries/tetrahedra_3d_4.h
double detJMX = s10 * y20 * z30 + y10 * z20 * s30 + z10 * s20 * y30 - s30 * y20 * z30 - y30 * z20 * s10 - z30 * s20 * y10;
double detJMY = s10 * x20 * z30 + x10 * z20 * s30 + z10 * s20 * x30 - s30 * x20 * z30 - x30 * z20 * s10 - z30 * s20 * x10;
double detJMZ = s10 * x20 * y30 + x10 * y20 * s30 + y10 * s20 * x30 - s30 * x20 * y30 - x30 * y20 * s10 - y30 * s20 * x10;
double detJMX = s10 * y20 * z30 + y10 * z20 * s30 + z10 * s20 * y30 - s30 * y20 * z10 - y30 * z20 * s10 - z30 * s20 * y10;
Copy link
Member

Choose a reason for hiding this comment

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

I should not change this! The volume results to be correct after my issue. Why you want to change it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not the volume function. This calculates the circumradius of the tetrahedron for the quality function that need them. It had an error in one of the components of a diagonal that calculates another determinant. See ac707a5.

But I can rewrite the whole matrix so it looks like the volume if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Now I see. So I aree with changes.

@roigcarlo
Copy link
Member Author

Can we merge?

@pooyan-dadvand pooyan-dadvand merged commit df10892 into master Mar 10, 2017
@roigcarlo roigcarlo deleted the feature-fix-quality-metrics branch March 13, 2017 08:49
@roigcarlo roigcarlo removed this from C++ in Kratos Testing May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RiccardoRossi RiccardoRossi RiccardoRossi left review comments

@pooyan-dadvand pooyan-dadvand pooyan-dadvand approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some quality functions not working properly with non-unitary geometries failure in the c++ tests
4 participants
@roigcarlo @loumalouomega @RiccardoRossi @pooyan-dadvand