tagging better #903
tagging better #903
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/amr/tagging/default_hybrid_tagger_strategy.hpp (1)
111-114: Consider refactoringfield_difffor better readabilityTo improve code readability, consider assigning intermediate calculations to named variables within the
field_difflambda function. This will make the code easier to understand and maintain.Apply the following diff to refactor the code:
auto field_diff = [&](auto const& F) // { + auto delta_x = F(ix + 2, iy) - F(ix, iy); + auto delta_x_prev = F(ix + 1, iy) - F(ix, iy); + auto criter_x = std::abs(delta_x) / (1 + std::abs(delta_x_prev)); + + auto delta_y = F(ix, iy + 2) - F(ix, iy); + auto delta_y_prev = F(ix, iy + 1) - F(ix, iy); + auto criter_y = std::abs(delta_y) / (1 + std::abs(delta_y_prev)); + - return std::make_tuple(std::abs((F(ix + 2, iy) - F(ix, iy)) - / (1 + std::abs(F(ix + 1, iy) - F(ix, iy)))), - std::abs(F(ix, iy + 2) - F(ix, iy)) - / (std::abs(F(ix, iy + 1) - F(ix, iy)) + 1)); + return std::make_tuple(criter_x, criter_y); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/amr/tagging/default_hybrid_tagger_strategy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/cmake_ubuntu.yml (1 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp
🧰 Additional context used
📓 Learnings (1)
.github/workflows/cmake_ubuntu.yml (1)
Learnt from: PhilipDeegan PR: PHAREHUB/PHARE#898 File: .github/workflows/cmake_ubuntu.yml:81-85 Timestamp: 2024-10-15T16:27:04.609Z Learning: In this project, the `RelWithDebInfo` build type includes `-DNDEBUG`, which disables asserts. To keep asserts enabled while having some optimizations, the `Debug` build type is used with the `-O3` optimization flag.
🔇 Additional comments (1)
.github/workflows/cmake_ubuntu.yml (1)
77-77: Approved: SAMRAI build type change to DebugChanging the SAMRAI build type from RelWithDebInfo to Debug aligns with the project's goal of keeping asserts enabled. This is beneficial for debugging and catching potential issues during development.
However, it's important to monitor the performance impact of this change. Consider running the following command to compare build times:
| make -j2 && sudo make install && cd ../.. && rm -rf samrai | ||
| cd ${{runner.workspace}}/build && rm -rf * | ||
| cmake $GITHUB_WORKSPACE -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON --fresh \ | ||
| -DCMAKE_BUILD_TYPE=RelWithDebInfo -Dasan=OFF \ | ||
| -DCMAKE_C_COMPILER_LAUNCHER=ccache \ | ||
| -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \ | ||
| -DlowResourceTests=ON -DdevMode=ON -Dbench=ON \ | ||
| -DCMAKE_CXX_FLAGS="-DPHARE_DIAG_DOUBLES=1 " -Dphare_configurator=ON | ||
| -DCMAKE_CXX_FLAGS="-O3 -DPHARE_DIAG_DOUBLES=1 " -Dphare_configurator=ON |
There was a problem hiding this comment.
Reconsider the combination of Debug build type with -O3 optimization
The addition of the -O3 flag to CMAKE_CXX_FLAGS while using Debug build type is unusual and potentially problematic. This combination can lead to unexpected behavior as Debug builds typically include minimal optimizations to aid in debugging, while -O3 applies aggressive optimizations.
Consider one of the following alternatives:
- Remove the -O3 flag to maintain a true Debug build:
- -DCMAKE_CXX_FLAGS="-O3 -DPHARE_DIAG_DOUBLES=1 " -Dphare_configurator=ON
+ -DCMAKE_CXX_FLAGS="-DPHARE_DIAG_DOUBLES=1 " -Dphare_configurator=ON- Create a custom build type that enables both debugging symbols and optimizations:
- -DCMAKE_BUILD_TYPE=RelWithDebInfo -Dasan=OFF \
+ -DCMAKE_BUILD_TYPE=CustomDebug -Dasan=OFF \
+ -DCMAKE_CXX_FLAGS_CUSTOMDEBUG="-g -O2 -DPHARE_DIAG_DOUBLES=1" \This approach allows you to define a build type that suits your specific needs.
Committable suggestion was skipped due to low confidence.
fix tagging (missing absolute value)
fix tagging (missing absolute value)
This PR fixes the tagging formula by adding the missing absolute value at the denominator.
Results below were obtained with this diff on top of #893
current master formula (also in the paper) leads to this refinement if increasing the threshold
clearly the two current layers being similar in terms of scales and magnetic jump they should be refined the same way.
This comes from the absolute value missing in the denominator that makes the positive and negative derivatives of B leading to different values of tagging formula hence difference response to the thresholding.
adding the absolute value and testing it for different tagging thresholds leads to
Which behaves better, see the pic above that tested the same configuration as above for different thresholds:
Summary by CodeRabbit
Bug Fixes
New Features