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

CylBinaryCompactObject: add equiangular map. #4697

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

markscheel
Copy link
Contributor

@markscheel markscheel commented Feb 6, 2023

Proposed changes

Adds an equiangular map option to CylindricalBinaryCompactObject.

Upgrade instructions

When using the CylindricalBinaryCompactObject domain creator one must now specify UseEquiangularMap. Setting this option to false gives the previous behavior.

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

Are you relatively certain that the unit tests would catch bugs where coord maps join discontinuously because of missing equiangular maps? For example, do they fail when you only make some of the cylinders equiangular?

@markscheel
Copy link
Contributor Author

markscheel commented Feb 9, 2023

I tried making only some of the wedges equiangular, or some of the cylinders equiangular, and the test still passes. I don't understand why TestHelpers::domain::creators::test_domain_creator doesn't test this. What happens for the other BBH domain? Is that untested as well?

Made other changes except for cartesian_product.

@kidder
Copy link
Member

kidder commented Feb 9, 2023

I tried making only some of the wedges equiangular, or some of the cylinders equiangular, and the test still passes. I don't understand why TestHelpers::domain::creators::test_domain_creator doesn't test this. What happens for the other BBH domain? Is that untested as well?

Made other changes except for cartesian_product.

try cherry-picking the commit from https://github.com/kidder/spectre/tree/check_block_faces and see if the test still passes

@markscheel
Copy link
Contributor Author

markscheel commented Feb 9, 2023

Rebased, which also did some of the requested changes here. So now there are only 3 relevant fixup commits.
I will try https://github.com/kidder/spectre/tree/check_block_faces

@markscheel
Copy link
Contributor Author

markscheel commented Feb 9, 2023

Thanks @kidder ! when I cherry-pick https://github.com/kidder/spectre/tree/check_block_faces, the current PR passes the tests as-is, but if I intentionally introduce a bug by turning on use_equiangular_map for some blocks but not others, then the test fails.

@kidder
Copy link
Member

kidder commented Feb 9, 2023

okay, I'll clean that up and make a PR

nilsvu
nilsvu previously approved these changes Feb 9, 2023
Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

I thought that continuity of maps was tested in one of the generic tests already, but apparently it isn't. Thanks for checking. We don't need to hold up the PR with that. @kidder thanks for adding that test.

@kidder
Copy link
Member

kidder commented Feb 9, 2023

the current test only checks the corners of the block faces, not the interior...

@markscheel
Copy link
Contributor Author

squashed

Copy link
Member

@nilsvu nilsvu left a comment

Choose a reason for hiding this comment

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

The test timed out in one of the builds. I've seen timeouts in this test before. Can we reduce the amount of work done in the big cartesian product of options?
Edit: or we randomly skip some of the iterations of the loop ("fuzzy cartesian product")

nilsvu
nilsvu previously approved these changes Feb 11, 2023
@nilsvu
Copy link
Member

nilsvu commented Feb 11, 2023

@markscheel would you mind adding the random-skipping code also to the BinaryCompactObject domain test while you rebase/squash? It's been timing out as well.

geoffrey4444
geoffrey4444 previously approved these changes Feb 12, 2023
Previously was using `typename CenterA::type center_A`, etc.
The idea is to speed up the test by not testing
every possible combination of the options.
@nilsvu nilsvu merged commit 2e9d67e into sxs-collaboration:develop Feb 14, 2023
@markscheel markscheel deleted the CylEquiangularMap branch February 14, 2023 16:27
@nilsvu nilsvu mentioned this pull request Feb 19, 2023
3 tasks
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

4 participants