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

improve approval tester readabillity. #809

Merged

MFraters merged 2 commits into GeodynamicWorldBuilder:main from MFraters:improve_approval_test_readablility

Jan 23, 2025

Member

MFraters commented Jan 21, 2025

This adds the infrastructure to make make the approval tests more readable and to connect individual entries more easily to the code. It applies this for some, but not all approval tests.

github-actions bot commented Jan 21, 2025 •

edited

Loading

Benchmark	Main	Feature	Difference (99.9% CI)
Slab interpolation simple none	1.112 ± 0.009 (s=423)	1.112 ± 0.009 (s=389)	-0.2% .. +0.2%
Slab interpolation curved simple none	1.113 ± 0.009 (s=404)	1.113 ± 0.009 (s=407)	-0.2% .. +0.2%
Spherical slab interpolation simple none	1.093 ± 0.011 (s=422)	1.093 ± 0.012 (s=404)	-0.3% .. +0.2%
Slab interpolation simple curved CMS	1.167 ± 0.014 (s=404)	1.166 ± 0.012 (s=370)	-0.3% .. +0.2%
Spherical slab interpolation simple CMS	1.444 ± 0.015 (s=318)	1.440 ± 0.012 (s=309)	-0.5% .. -0.0%
Spherical fault interpolation simple none	1.100 ± 0.008 (s=432)	1.100 ± 0.009 (s=389)	-0.1% .. +0.2%
Cartesian min max surface	2.536 ± 0.015 (s=177)	2.535 ± 0.015 (s=181)	-0.2% .. +0.2%
Spherical min max surface	7.161 ± 0.077 (s=70)	7.172 ± 0.069 (s=58)	-0.5% .. +0.8%

MFraters force-pushed the improve_approval_test_readablility branch from 8a206ce to b0aec7d Compare

January 22, 2025 13:47

gassmoeller approved these changes

View reviewed changes

Contributor

gassmoeller left a comment

As discussed I think this makes a lot of sense especially if you add names to all new tests. I have a suggestion for how to impact your existing results less, take a look what you think about it.

tests/unit_tests/unit_test_world_builder.cc Outdated

    
                for (auto&& value : approval_tests)

                  {

                    std::stringstream s;

                    s << value.first << " - " << value.second;

Contributor

gassmoeller Jan 23, 2025

This leads to a lot of - signs without a preceding name, which looks a bit like a minus sign and messes up your existing approval results. What do you think about the following?

Suggested change

      
                  s << value.first << " - " << value.second;
          
                  s << value.first << (value.first != "") ? " - " : "" << value.second;

tests/unit_tests/unit_test_world_builder.cc Outdated

    
                for (auto&& value : approval_tests)

                  {

                    std::stringstream s;

                    s << value.first << " - " << value.second;

Contributor

gassmoeller Jan 23, 2025

see comment above

tests/unit_tests/unit_test_world_builder.cc Outdated

    
                for (auto&& value : approval_tests)

                  {

                    std::stringstream s;

                    s << value.first << " - " << value.second;

Contributor

gassmoeller Jan 23, 2025

and here

tests/unit_tests/unit_test_world_builder.cc Outdated

    
                for (auto&& value : approval_tests)

                  {

                    std::stringstream s;

                    s << value.first << " - " << value.second;

Contributor

gassmoeller Jan 23, 2025

and here

tests/unit_tests/unit_test_world_builder.cc Outdated

    
                for (auto&& value : approval_tests_grains)

                  {

                    std::stringstream s;

                    s << value.first << " - " << value.second;

Contributor

gassmoeller Jan 23, 2025

and all other occurences of " - "

MFraters force-pushed the improve_approval_test_readablility branch 2 times, most recently from eec20e1 to d732821 Compare

January 23, 2025 13:32

MFraters added 2 commits

January 23, 2025 14:33


          improve approval tester readabillity.

d9ec9af


          Add changelog entry for improved approval tester readability

e069010

MFraters force-pushed the improve_approval_test_readablility branch from d732821 to e069010 Compare

January 23, 2025 13:34

gassmoeller approved these changes

View reviewed changes

Contributor

gassmoeller left a comment

Yes, looks good. Ready to merge if you are happy with the current state.

gassmoeller added the ready to merge label

MFraters merged commit 4df60e5 into GeodynamicWorldBuilder:main

37 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels