spatially variable hyperresistivity #893
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new integer parameter Changes
Possibly related PRs
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: 4
Outside diff range and nitpick comments (1)
src/core/numerics/ohm/ohm.hpp (1)
341-376: Simplify computation ofdl2usingstd::inner_productThe calculation of
dl2can be made more concise and expressive by usingstd::inner_productinstead ofstd::accumulatewith a custom lambda.Apply this diff to improve the code:
- double const dl2{std::accumulate(std::begin(layout_->meshSize()), - std::end(layout_->meshSize()), 0., - [](double acc, double d) { return acc + d * d; })}; + double const dl2{std::inner_product(std::begin(layout_->meshSize()), + std::end(layout_->meshSize()), + std::begin(layout_->meshSize()), + 0.0)};
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- src/core/data/grid/gridlayout.hpp (3 hunks)
- src/core/data/grid/gridlayoutimplyee.hpp (3 hunks)
- src/core/data/tensorfield/tensorfield.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (5 hunks)
Files skipped from review due to trivial changes (2)
- src/core/data/tensorfield/tensorfield.hpp
- src/core/data/vecfield/vecfield.hpp
Additional context used
Path-based instructions (3)
src/core/data/grid/gridlayout.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/grid/gridlayoutimplyee.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (8)
src/core/data/grid/gridlayout.hpp (3)
1076-1076: LGTM!The
BxToExfunction is a straightforward wrapper that delegates to the corresponding function inGridLayoutImpl. The implementation looks good.
1100-1100: LGTM!The
ByToEyfunction is a straightforward wrapper that delegates to the corresponding function inGridLayoutImpl. The implementation looks good.
1124-1124: LGTM!The
BzToEzfunction is a straightforward wrapper that delegates to the corresponding function inGridLayoutImpl. The implementation looks good.src/core/numerics/ohm/ohm.hpp (3)
14-15: Inclusion of<numeric>header is appropriateThe
<numeric>header is required for the use ofstd::accumulatein the code below.
82-83: Correct integration ofhyperresistive_term intoE_Eq_computationThe addition of the
hyperresistive_term enhances the physical modeling capabilities and aligns with the PR objectives.
285-286: Incorrect placement of#include <cmath>Including the
<cmath>header inside a function or code block is invalid. Headers should be included at the top of the file, outside any namespace or function.Apply this diff to move the include statement:
// Add at the top of the file, after existing includes: +#include <cmath> // Remove the misplaced include inside the function: -#include <cmath>Likely invalid or redundant comment.
src/core/data/grid/gridlayoutimplyee.hpp (2)
788-828: Implementation ofBzToEz()is correctThe
BzToEz()function correctly calculates the weight points for transforming Bz to Ez in 1D, 2D, and 3D cases. The logic and coefficients are appropriate.
713-725: Fix missing variable names in WeightPoint declarations for dimension == 3 inBxToEx()In the
BxToEx()function, within thedimension == 3branch, theWeightPointdeclarations forP6,P7, andP8are missing variable names, leading to syntax errors. Eachconstexpr WeightPoint<dimension>declaration should include a variable name.Apply the following diff to correct the declarations:
constexpr WeightPoint<dimension> P5{Point<int, dimension>{0, 0, d2pShift}, 0.125}; - constexpr WeightPoint<dimension>{0, d2pShift, d2pShift}, 0.125}; + constexpr WeightPoint<dimension> P6{Point<int, dimension>{0, d2pShift, d2pShift}, 0.125}; - constexpr WeightPoint<dimension>{p2dShift, 0, d2pShift}, 0.125}; + constexpr WeightPoint<dimension> P7{Point<int, dimension>{p2dShift, 0, d2pShift}, 0.125}; - constexpr WeightPoint<dimension>{p2dShift, d2pShift, d2pShift}, 0.125}; + constexpr WeightPoint<dimension> P8{Point<int, dimension>{p2dShift, d2pShift, d2pShift}, 0.125};Likely invalid or redundant comment.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/core/data/grid/gridlayout.hpp (2)
115-123: LGTM: Constructor updated to include level number.The addition of the
level_numberparameter and its initialization is a good improvement for supporting multi-level simulations or AMR.Consider adding a brief comment explaining the purpose of the
level_numberparameter for better code documentation.
Line range hint
1077-1126: LGTM: Added B to E projection methods.The new
BxToEx(),ByToEy(), andBzToEz()methods are good additions, providing access to layout-specific projection information. The implementation correctly delegates toGridLayoutImpl.For consistency with other similar methods in the class, consider adding brief documentation comments for each of these new methods, explaining their purpose and return value.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/amr/resources_manager/amr_utils.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/numerics/ohm/ohm.hpp (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/numerics/ohm/ohm.hpp
🧰 Additional context used
📓 Path-based instructions (2)
src/amr/resources_manager/amr_utils.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/grid/gridlayout.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (3)
src/amr/resources_manager/amr_utils.hpp (1)
185-186: LGTM! Consider verifying impact and updating documentation.The addition of the
lvlNbrparameter to theGridLayoutTconstructor enhances the object's functionality by including patch level information. This change aligns well with the PR objectives.To ensure this change doesn't introduce any issues:
- Verify that all other uses of the
GridLayoutTconstructor in the codebase have been updated to include the newlvlNbrparameter.- Check if any existing documentation for
GridLayoutTneeds to be updated to reflect this change.Consider adding a brief comment explaining the purpose of including the patch level number in the
GridLayoutTobject, if it's not already documented elsewhere.src/core/data/grid/gridlayout.hpp (2)
1174-1174: LGTM: Added getter for level number.The
levelNumber()method is a good addition, providing controlled access to thelevelNumber_member variable.
1518-1519: LGTM: Added level number member variable.The addition of the
levelNumber_member variable is consistent with the other changes and provides necessary storage for the level number in multi-level simulation scenarios.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/core/numerics/ohm/ohm.hpp (3)
31-33: Consider improving readability ofhyper_modeinitialization.The initialization of
hyper_modeis correct, but the use of a ternary operator in the member initializer list can be hard to read. Consider moving this logic to a separate function for improved clarity:- , hyper_mode{cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant" - ? HyperMode::constant - : HyperMode::spatial} + , hyper_mode{getHyperMode(dict)} + { + } + + static HyperMode getHyperMode(PHARE::initializer::PHAREDict const& dict) + { + auto mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"}); + return mode == "constant" ? HyperMode::constant : HyperMode::spatial; + }This change would improve readability and make the constructor easier to understand at a glance.
358-362: Address TODO and add explanatory comment forconstant_hyperresistive_.The method has been updated to align with the changes in
hyperresistive_. However, there are two points to consider:
The TODO comment references an open issue. It would be helpful to add more context about what needs to be done or why this is flagged as a TODO.
Consider adding a brief comment explaining the calculation being performed. This will help future maintainers understand the purpose and logic of this method.
Example:
template<auto component, typename VecField> auto constant_hyperresistive_(VecField const& J, MeshIndex<VecField::dimension> index) const -{ // TODO : https://github.com/PHAREHUB/PHARE/issues/3 +{ + // TODO: Implement more efficient calculation method (see https://github.com/PHAREHUB/PHARE/issues/3) + // Calculate the constant hyperresistive term using the Laplacian of the current density return -nu_ * layout_->laplacian(J(component), index); }
293-293: Move<cmath>include to the top of the file.The
<cmath>header is correctly included for thestd::sqrtfunction used in thespatial_hyperresistive_method. However, it's unconventional to place include statements in the middle of a file. For better readability and adherence to coding standards, move this include to the top of the file with the other includes.#include "initializer/data_provider.hpp" #include <numeric> +#include <cmath> namespace PHARE::core { // ... (rest of the file) -#include <cmath> // ... (rest of the file)This change will improve the organization of the file and make it easier for developers to see all dependencies at a glance.
src/core/data/grid/gridlayoutimplyee.hpp (3)
688-728: Approved: Well-implemented transformation functionThe
BxToExfunction is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from Bx (primal dual dual) to Ex (dual primal primal). The use ofconstexprandstaticis appropriate for compile-time optimization.Consider adding a brief comment explaining the meaning of the weight values (0.5, 0.25, 0.125) for better code readability. For example:
// Weight 0.5 for 1D, 0.25 for 2D, 0.125 for 3D due to averaging over additional dimensions constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.125;
788-828: Approved: Correct implementation with room for consistency improvementThe
BzToEzfunction is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from Bz (dual dual primal) to Ez (primal primal dual). The use ofconstexprandstaticis appropriate for compile-time optimization.For consistency with the
BxToExfunction, consider using a single constexpr variable for the weight calculation:constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.25;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors. This approach would improve readability and make it easier to modify weights in the future if needed.
Line range hint
688-962: Overall assessment: Well-implemented with room for consistency improvementsThe newly added functions
BxToEx,BzToEz, andByToEyare generally well-implemented and provide important functionality for field transformations in the Yee grid layout. They correctly handle different dimensions and use appropriate C++ features for optimization.To improve consistency and readability across these functions, consider the following refactoring:
- Introduce a helper function to calculate weights based on dimension:
constexpr double calculateWeight(std::size_t dimension) { return (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.125; }
Use this helper function in all three transformation functions instead of hard-coded values.
Consider creating a template function for generating
WeightPointarrays to reduce code duplication across the different dimension cases.These changes would make the code more maintainable and easier to understand, while also reducing the likelihood of inconsistencies between similar functions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (1 hunks)
- src/amr/resources_manager/amr_utils.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/data/grid/gridlayoutimplyee.hpp (3 hunks)
- src/core/data/tensorfield/tensorfield.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pyphare/pyphare/pharein/init.py
- pyphare/pyphare/pharein/simulation.py
- src/amr/resources_manager/amr_utils.hpp
- src/core/data/grid/gridlayout.hpp
- src/core/data/tensorfield/tensorfield.hpp
- src/core/data/vecfield/vecfield.hpp
🧰 Additional context used
📓 Path-based instructions (2)
src/core/data/grid/gridlayoutimplyee.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (2)
src/core/numerics/ohm/ohm.hpp (2)
19-20: LGTM: Well-defined enum for hyperresistivity modes.The addition of the
HyperModeenum class is a good practice. It provides type safety and clearly defines the available modes for hyperresistivity calculations.
14-15: Verify the necessity of the<numeric>include.The
<numeric>header has been added, but there doesn't appear to be any direct use of its functions in the visible changes. Please ensure that this include is necessary, possibly for code not shown in this diff.To check if this include is used elsewhere in the file or project, you can run:
If no results are found, consider removing this include to keep the header dependencies minimal.
✅ Verification successful
Remove the unnecessary
<numeric>include.The
<numeric>header is not used insrc/core/numerics/ohm/ohm.hpp. Please remove this include to minimize dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uses of functions from <numeric> in the current file and project echo "Searching in current file:" rg --type cpp -e 'std::(accumulate|reduce|inner_product|adjacent_difference|partial_sum|iota)' src/core/numerics/ohm/ohm.hpp echo "\nSearching in entire project:" rg --type cpp -e 'std::(accumulate|reduce|inner_product|adjacent_difference|partial_sum|iota)'Length of output: 3836
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/core/numerics/ohm/ohm.hpp (1)
31-33: LGTM: Clear initialization of hyper_modeThe initialization of
hyper_modeis well-implemented. It correctly usescppdict::get_valueto provide a default value of "constant" if the key is not present in the dictionary.For improved readability, consider using an if-else statement instead of the ternary operator:
- , hyper_mode{cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant" - ? HyperMode::constant - : HyperMode::spatial} + , hyper_mode{[&dict]() { + auto mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"}); + if (mode == "constant") + return HyperMode::constant; + else if (mode == "spatial") + return HyperMode::spatial; + else + throw std::invalid_argument("Invalid hyper_mode value"); + }()}This change would also add error checking for invalid
hyper_modevalues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/core/numerics/ohm/ohm.hpp (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (3)
src/core/numerics/ohm/ohm.hpp (3)
19-20: LGTM: Well-defined enum class for HyperModeThe addition of the
HyperModeenum class is a good practice. It provides clear, type-safe options for the hyperresistivity mode, which enhances code readability and prevents errors from using incorrect values.
344-354:⚠️ Potential issueImprove error handling in hyperresistive_ method
The method now correctly uses the
hyper_modeto determine the calculation method. However, theelseclause that returns 0 could hide potential errors. Consider the following improvements:
- Make the
elsecase unreachable by design:if (hyper_mode == HyperMode::constant) return constant_hyperresistive_<component>(J, index); - else if (hyper_mode == HyperMode::spatial) + else // hyper_mode == HyperMode::spatial return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - return 0.;
- If you want to keep the
elsecase for future extensibility, throw an exception instead of returning 0:if (hyper_mode == HyperMode::constant) return constant_hyperresistive_<component>(J, index); else if (hyper_mode == HyperMode::spatial) return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - return 0.; + else + throw std::runtime_error("Invalid HyperMode");These changes will ensure that unexpected cases are properly handled and don't silently return incorrect results.
Likely invalid or redundant comment.
14-15: Verify the necessity of the headerThe
<numeric>header has been added, but it's not immediately clear where functions from this header are used in the visible code. Could you please clarify the reason for including this header? If it's not being used, consider removing it to keep the includes minimal.✅ Verification successful
Remove unnecessary
<numeric>headerNo functions from the
<numeric>header are used insrc/core/numerics/ohm/ohm.hpp. Consider removing this include to keep the dependencies minimal.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any functions from <numeric> are used in the file rg --type cpp -i '\b(accumulate|adjacent_difference|inner_product|partial_sum|iota|reduce|transform_reduce)\b' src/core/numerics/ohm/ohm.hppLength of output: 142
Script:
#!/bin/bash # List all #include directives in src/core/numerics/ohm/ohm.hpp rg '^#include\s+<[^>]+>' src/core/numerics/ohm/ohm.hpp # Search for any numerical operations that might implicitly use <numeric> rg --type cpp -i '\b(accumulate|adjacent_difference|inner_product|partial_sum|iota|reduce|transform_reduce|std::accumulate|std::inner_product)\b' src/core/numerics/ohm/ohm.hppLength of output: 309
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/core/numerics/ohm/ohm.hpp (2)
32-34: Consider improving readability and error handling in the constructor.While the current initialization of
hyper_modeis functional, it could be improved for better readability and error handling. Consider using an if-else structure or a separate function for clarity. Also, it might be beneficial to provide a default value or throw an exception if the "hyper_mode" key is missing from the dictionary.Example:
hyper_mode{getHyperMode(dict)} // ... HyperMode getHyperMode(const PHARE::initializer::PHAREDict& dict) { auto mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"}); if (mode == "constant") return HyperMode::constant; if (mode == "spatial") return HyperMode::spatial; throw std::invalid_argument("Invalid hyper_mode value"); }
294-294: Move the include to the top of the file.The
<cmath>header has been added in an unusual location within the file. While it's needed for thestd::sqrtfunction used in thespatial_hyperresistive_method, it's conventional and more maintainable to place all include statements at the top of the file.Please move this include statement to join the other includes at the beginning of the file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/core/numerics/ohm/ohm.hpp (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
src/core/numerics/ohm/ohm.hpp (4)
14-16: LGTM: New includes added appropriately.The addition of
<numeric>and<algorithm>headers is appropriate, likely to support new functionality in the class. These standard library headers provide useful algorithms and functions that can improve code quality and performance.
20-21: LGTM: New enum classHyperModeadded.The introduction of the
HyperModeenum class is a good design choice. It provides type-safe options for selecting between constant and spatial hyperresistivity modes, enhancing code readability and reducing the likelihood of errors.
359-363: LGTM:constant_hyperresistive_method updated correctly.The method signature has been appropriately updated to align with the new
hyperresistive_method. The implementation remains correct and unchanged.
345-355:⚠️ Potential issueImprove error handling in
hyperresistive_method.The method now correctly uses the
hyper_modeto determine the calculation method. However, theelseclause that returns 0 could hide potential errors. Consider the following improvements:
- Make the
elsecase unreachable by design:if (hyper_mode == HyperMode::constant) return constant_hyperresistive_<component>(J, index); - else if (hyper_mode == HyperMode::spatial) + else // hyper_mode == HyperMode::spatial return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - return 0.;
- If you want to keep the
elsecase for future extensibility, throw an exception instead of returning 0:if (hyper_mode == HyperMode::constant) return constant_hyperresistive_<component>(J, index); else if (hyper_mode == HyperMode::spatial) return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - return 0.; + else + throw std::runtime_error("Invalid HyperMode");These changes will ensure that unexpected cases are properly handled and don't silently return incorrect results.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/core/numerics/ohm/ohm.hpp (2)
31-33: LGTM with a minor suggestion for readability.The initialization of the
hyper_modemember variable is correct and provides a default value. However, for improved readability, consider using an if-else statement instead of a ternary operator, especially if more modes are added in the future., hyper_mode{[&dict]() { auto mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"}); if (mode == "constant") return HyperMode::constant; else if (mode == "spatial") return HyperMode::spatial; else throw std::invalid_argument("Invalid hyper_mode value"); }()}This approach allows for easier extension and better error handling.
358-362: LGTM: Minor signature update inconstant_hyperresistive_.The method signature has been updated to match the new usage in
hyperresistive_. The implementation remains unchanged, which is correct for the constant mode.Don't forget to address the TODO comment referencing issue #3 when appropriate.
src/core/data/grid/gridlayout.hpp (2)
115-123: LGTM. Consider adding documentation for the new parameter.The addition of the
level_numberparameter to the constructor is well-implemented and maintains backwards compatibility.Consider adding a brief comment explaining the purpose of the
level_numberparameter in the constructor's documentation.
1174-1174: LGTM. Consider using an explicit return type.The
levelNumber()method is correctly implemented as a const member function.Consider using an explicit return type instead of
autofor clarity:-auto levelNumber() const { return levelNumber_; } +int levelNumber() const { return levelNumber_; }src/core/data/grid/gridlayoutimplyee.hpp (2)
688-728: Approved: Well-implemented transformation functionThe
BxToExfunction is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from Bx (primal dual dual) to Ex (dual primal primal). The use ofconstexprandstaticis appropriate for compile-time optimization.Consider using a single
constexprvariable for the weight calculation to improve readability:constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.125;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors.
788-828: Approved: Consistent implementation with BxToExThe
BzToEzfunction is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from Bz (dual dual primal) to Ez (primal primal dual). The implementation is consistent with theBxToExfunction, which is good for code maintainability.For consistency with the previous suggestion, consider using a single
constexprvariable for the weight calculation:constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.25;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (2 hunks)
- src/amr/resources_manager/amr_utils.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/data/grid/gridlayoutimplyee.hpp (3 hunks)
- src/core/data/tensorfield/tensorfield.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pyphare/pyphare/pharein/init.py
- pyphare/pyphare/pharein/simulation.py
- src/amr/resources_manager/amr_utils.hpp
- src/core/data/tensorfield/tensorfield.hpp
- src/core/data/vecfield/vecfield.hpp
🧰 Additional context used
📓 Path-based instructions (3)
src/core/data/grid/gridlayout.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/grid/gridlayoutimplyee.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
src/core/numerics/ohm/ohm.hpp (2)
19-20: LGTM: HyperMode enum addition.The addition of the
HyperModeenum is a good choice for type-safe selection of hyperresistivity modes. It improves code readability and prevents potential errors from using incorrect values.
344-354:⚠️ Potential issueImprove error handling in
hyperresistive_method.The method now correctly uses the
hyper_modeto determine the calculation method. However, theelseclause that returns 0 could hide potential errors. Consider the following improvements:
- Make the
elsecase unreachable by design:if (hyper_mode == HyperMode::constant) return constant_hyperresistive_<component>(J, index); - else if (hyper_mode == HyperMode::spatial) + else // hyper_mode == HyperMode::spatial return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - return 0.;
- If you want to keep the
elsecase for future extensibility, throw an exception instead of returning 0:if (hyper_mode == HyperMode::constant) return constant_hyperresistive_<component>(J, index); else if (hyper_mode == HyperMode::spatial) return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - return 0.; + else + throw std::runtime_error("Invalid HyperMode");These changes will ensure that unexpected cases are properly handled and don't silently return incorrect results.
Likely invalid or redundant comment.
src/core/data/grid/gridlayout.hpp (2)
Line range hint
1077-1126: LGTM. B to E projection methods are well-implemented.The new static constexpr methods for B to E field projections (
BxToEx(),ByToEy(),BzToEz()) are correctly implemented and consistent with the existing codebase.These methods enhance the functionality of the
GridLayoutclass by providing necessary projections for magnetic field components onto electric field components.
1518-1519: LGTM. New member variable is correctly declared and initialized.The
levelNumber_member variable is properly declared as anintand initialized to 0, which is consistent with the constructor's default parameter.This addition allows the
GridLayoutclass to keep track of its level in a potential multi-level grid structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
src/core/numerics/ohm/ohm.hpp (2)
27-29: Consider improving readability ofhyper_modeinitialization.The current initialization is concise but could be made more readable. Consider using an if-else statement or extracting the logic to a separate function.
- , hyper_mode{cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant" - ? HyperMode::constant - : HyperMode::spatial} + , hyper_mode{initHyperMode(dict)} + + // ... (in the class definition) + static HyperMode initHyperMode(const PHARE::initializer::PHAREDict& dict) { + auto mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"}); + return (mode == "constant") ? HyperMode::constant : HyperMode::spatial; + }This change would improve readability and make the logic easier to understand and maintain.
340-349: LGTM: Good implementation of hyperresistivity modes.The updated
hyperresistive_method effectively uses thehyper_modeto determine the calculation method. The error handling for an unknown mode is a good practice.Consider simplifying the conditional structure:
- if (hyper_mode == HyperMode::constant) - return constant_hyperresistive_<component>(J, index); - else if (hyper_mode == HyperMode::spatial) - return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - throw std::runtime_error("Error - Ohm - unknown hyper_mode"); + switch (hyper_mode) { + case HyperMode::constant: + return constant_hyperresistive_<component>(J, index); + case HyperMode::spatial: + return spatial_hyperresistive_<component>(J, B, n, index); + default: + throw std::runtime_error("Error - Ohm - unknown hyper_mode"); + }This change would make the code more readable and easier to extend if new modes are added in the future.
src/core/data/grid/gridlayout.hpp (2)
1174-1174: LGTM: New levelNumber accessor method added.The
levelNumber()method is correctly implemented as a const member function, providing read-only access to thelevelNumber_member. This is good for encapsulation.Consider explicitly specifying the return type for clarity:
-auto levelNumber() const { return levelNumber_; } +int levelNumber() const { return levelNumber_; }
Line range hint
1077-1126: LGTM: New B to E projection methods added.The addition of static constexpr methods
BxToEx(),ByToEy(), andBzToEz()for B to E field projections is well-implemented. The delegation toGridLayoutImplmethods suggests good design for layout-specific implementations.Consider adding brief documentation comments for these new methods to explain their purpose and return values, similar to other methods in the class. For example:
/** * @brief BxToEx returns the indexes and associated coefficients to compute the linear * interpolation necessary to project Bx onto Ex. */ NO_DISCARD auto static constexpr BxToEx() { return GridLayoutImpl::BxToEx(); }src/core/data/grid/gridlayoutimplyee.hpp (4)
688-728: Approved with a minor optimization suggestionThe
BxToExfunction is well-implemented, correctly handling the transformation from Bx (primal dual dual) to Ex (dual primal primal) for all dimensions (1D, 2D, 3D). The use ofconstexpris appropriate for compile-time optimization.Consider defining a constant for the weight calculation to improve readability:
constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.125;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors.
788-828: Approved with refactoring suggestion for code reuseThe
BzToEzfunction is correctly implemented, handling the transformation from Bz (dual dual primal) to Ez (primal primal dual) for all dimensions (1D, 2D, 3D). The use ofconstexpris appropriate for compile-time optimization.Consider creating a template function to handle the common logic in
BxToEx,BzToEz, and potentially other similar functions. This would reduce code duplication and improve maintainability. For example:template<int ShiftX, int ShiftY, int ShiftZ> NO_DISCARD auto static constexpr TransformField() { auto constexpr p2dShift = primalToDual(); auto constexpr d2pShift = dualToPrimal(); constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.125; if constexpr (dimension == 1) { constexpr WeightPoint<dimension> P1{Point<int, dimension>{0}, weight}; constexpr WeightPoint<dimension> P2{Point<int, dimension>{ShiftX}, weight}; return std::array<WeightPoint<dimension>, 2>{P1, P2}; } else if constexpr (dimension == 2) { // ... (similar implementation for 2D) } else if constexpr (dimension == 3) { // ... (similar implementation for 3D) } }Then, you can use this template function in
BxToExandBzToEz:NO_DISCARD auto static constexpr BxToEx() { return TransformField<primalToDual(), dualToPrimal(), dualToPrimal>(); } NO_DISCARD auto static constexpr BzToEz() { return TransformField<dualToPrimal(), dualToPrimal(), primalToDual>(); }This approach would significantly reduce code duplication and make it easier to maintain and extend these functions in the future.
923-962: Approved with reiteration of refactoring suggestionThe
ByToEyfunction is correctly implemented, handling the transformation from By (dual primal dual) to Ey (primal dual primal) for all dimensions (1D, 2D, 3D). The use ofconstexpris appropriate for compile-time optimization.As mentioned in the previous comment, the similarity between
ByToEy,BxToEx, andBzToEzstrongly suggests the need for a template function to handle these transformations. This would significantly reduce code duplication and improve maintainability.Using the previously suggested
TransformFieldtemplate function, you could implementByToEyas:NO_DISCARD auto static constexpr ByToEy() { return TransformField<dualToPrimal(), primalToDual(), dualToPrimal>(); }This approach would make the code more concise, easier to maintain, and less prone to errors when implementing similar functions in the future.
Line range hint
1-962: Overall: Well-implemented with suggestions for improvementThe changes in this file successfully implement new projections of the magnetic field (B) onto the electric field (E), aligning with the PR objectives. The new functions (
BxToEx,BzToEz,ByToEy) are well-structured, correctly handle all dimensions, and useconstexprfor compile-time optimization.Main suggestions for improvement:
- Implement a template function to handle the common logic in these transformation functions, reducing code duplication and improving maintainability.
- Consider using a constant for weight calculations to improve readability.
These changes enhance the functionality of the
GridLayoutImplYeeclass, providing necessary tools for computing the norm of the magnetic field at specific E locations. The implementation is solid, and with the suggested refactoring, it will be even more robust and easier to maintain.Consider creating a separate file for these field transformation functions if the
GridLayoutImplYeeclass becomes too large. This would improve code organization and make it easier to test and maintain these functions independently.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (2 hunks)
- src/amr/resources_manager/amr_utils.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/data/grid/gridlayoutimplyee.hpp (3 hunks)
- src/core/data/tensorfield/tensorfield.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pyphare/pyphare/pharein/init.py
- pyphare/pyphare/pharein/simulation.py
- src/amr/resources_manager/amr_utils.hpp
- src/core/data/tensorfield/tensorfield.hpp
- src/core/data/vecfield/vecfield.hpp
🧰 Additional context used
📓 Path-based instructions (3)
src/core/data/grid/gridlayout.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/grid/gridlayoutimplyee.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
src/core/numerics/ohm/ohm.hpp (3)
15-15: LGTM: Clear and concise enumeration added.The
HyperModeenumeration is a good addition, providing a clear way to distinguish between constant and spatial hyperresistivity modes.
Line range hint
1-396: Overall LGTM: Good improvements to the Ohm class.The changes to the
Ohmclass, including the addition of theHyperModeenum, the newspatial_hyperresistive_method, and the updates to thehyperresistive_method, significantly enhance the functionality of the class. The implementation allows for more flexible hyperresistivity calculations based on the chosen mode.Some minor suggestions for improvement have been made, primarily focusing on readability and robustness. Additionally, it would be beneficial to get more context about the TODO comments and the plans for addressing issue #3.
Great work on implementing these new features!
360-390: LGTM with suggestions: New spatial hyperresistivity calculation.The new
spatial_hyperresistive_method is a good addition, providing a more complex calculation for the hyperresistive term. Here are some suggestions for improvement:
- Add a comment explaining the purpose of the
lvlCoeffcalculation.- Consider handling the case where
nOnEis zero to prevent potential division by zero errors.- The
TODOcomment suggests that there might be future changes planned. It would be helpful to have more context about this.Here's a suggested improvement for the lambda function:
auto computeHR = [&](auto BxProj, auto ByProj, auto BzProj, auto nProj) { auto const BxOnE = GridLayout::project(B(Component::X), index, BxProj); auto const ByOnE = GridLayout::project(B(Component::Y), index, ByProj); auto const BzOnE = GridLayout::project(B(Component::Z), index, BzProj); auto const nOnE = GridLayout::project(n, index, nProj); auto b = std::sqrt(BxOnE * BxOnE + ByOnE * ByOnE + BzOnE * BzOnE); + if (nOnE == 0) { + // Handle division by zero case + return 0.0; // or throw an exception, depending on your error handling strategy + } return -nu_ * b / nOnE * lvlCoeff * layout_->laplacian(J(component), index); };Could you provide more information about the plans for addressing issue #3? This might help in understanding the future direction of this method.
#!/bin/bash # Fetch the content of issue #3 gh issue view 3 --repo PHAREHUB/PHAREsrc/core/data/grid/gridlayout.hpp (1)
115-123: LGTM: Constructor updated correctly with new level number parameter.The addition of the
level_numberparameter with a default value of 0 is well-implemented. It maintains backward compatibility while allowing the specification of a level number when needed. The initialization oflevelNumber_in the constructor's initializer list is the correct approach.
| auto constant_hyperresistive_(VecField const& J, MeshIndex<VecField::dimension> index) const | ||
| { // TODO : https://github.com/PHAREHUB/PHARE/issues/3 | ||
| return -nu_ * layout_->laplacian(J(component), index); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
LGTM: Method signature updated correctly.
The constant_hyperresistive_ method signature has been updated to match the changes in the hyperresistive_ method, ensuring consistency in the class interface.
However, the method still unconditionally computes the hyperresistive term. According to issue #3, the implementation should conditionally include resistive and hyperresistive terms based on their coefficients.
Could you please update the method to align with the proposed design in issue #3 or provide an update on how and when this will be addressed?
🔗 Analysis chain
LGTM: Method signature updated correctly.
The constant_hyperresistive_ method signature has been updated to match the changes in the hyperresistive_ method. This ensures consistency in the class interface.
Regarding the TODO comment, could you provide more information about the plans for addressing issue #3? This might help in understanding the future direction of this method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the content of issue #3
gh issue view 3 --repo PHAREHUB/PHARE
Length of output: 1044
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
src/core/data/vecfield/vecfield.hpp (2)
Line range hint
35-55: LGTM with minor suggestions for improvement.The
VecFieldNamesstructure is well-designed and follows good C++ practices. Here are a few suggestions to further improve it:
Consider adding
constqualifiers to the string members if they're not intended to be modified after construction. This would make the immutability of these members explicit.The use of
explicitfor the templated constructor is good practice. It prevents unintended implicit conversions.You could modify the structure as follows:
struct VecFieldNames { const std::string vecName; const std::string xName; const std::string yName; const std::string zName; // ... rest of the code remains the same };
35-37: Add documentation for the VecFieldNames structure.To improve code readability and maintainability, consider adding documentation for the
VecFieldNamesstructure. This documentation should explain the purpose of the structure and how it's intended to be used.Here's a suggested documentation block:
/** * @brief Stores the names of a vector field and its components. * * This structure is used to encapsulate the names associated with a vector field, * including the name of the field itself and the names of its X, Y, and Z components. * It provides a convenient way to store and pass around these names as a single unit. */ struct VecFieldNames { // ... existing code ... };.github/workflows/cmake_ubuntu.yml (1)
81-85: Approved: Consistent build configuration with improved debuggingThe changes in the main CMake configuration are consistent with the SAMRAI subproject changes and align with the project's preference for keeping asserts enabled. The
Debugbuild type with-O3optimization maintains debugging capabilities while ensuring performance.Suggestion for clarity:
Consider separating the optimization flag from the project-specific flags inCMAKE_CXX_FLAGSfor better readability:- -DCMAKE_CXX_FLAGS="-O3 -DPHARE_DIAG_DOUBLES=1 " -Dphare_configurator=ON + -DCMAKE_CXX_FLAGS="-O3" -DPHARE_DIAG_DOUBLES=1 -Dphare_configurator=ONThis separation makes it easier to distinguish between compiler optimization flags and project-specific definitions.
src/core/numerics/ohm/ohm.hpp (2)
27-29: Consider using if-else for better readabilityThe current initialization of
hyper_modeuses a ternary operator, which is concise but can be less readable for complex conditions. Consider using an if-else statement for improved clarity.Here's a suggested refactor:
- , hyper_mode{cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant" - ? HyperMode::constant - : HyperMode::spatial} + , hyper_mode{[&dict]() { + auto mode = cppdict::get_value(dict, "hyper_mode", std::string{"constant"}); + if (mode == "constant") + return HyperMode::constant; + else if (mode == "spatial") + return HyperMode::spatial; + else + throw std::runtime_error("Invalid hyper_mode value"); + }()}This change also adds error handling for invalid mode values.
340-349: LGTM: hyperresistive_ method updateThe
hyperresistive_method has been successfully updated to accommodate both constant and spatial hyperresistivity calculations. The conditional structure and error handling are appropriate.Consider using a
switchstatement instead ofif-elsefor better readability and to leverage compiler optimizations:- if (hyper_mode == HyperMode::constant) - return constant_hyperresistive_<component>(J, index); - else if (hyper_mode == HyperMode::spatial) - return spatial_hyperresistive_<component>(J, B, n, index); - else // should not happen but otherwise -Wreturn-type fails with Werror - throw std::runtime_error("Error - Ohm - unknown hyper_mode"); + switch (hyper_mode) { + case HyperMode::constant: + return constant_hyperresistive_<component>(J, index); + case HyperMode::spatial: + return spatial_hyperresistive_<component>(J, B, n, index); + default: + throw std::runtime_error("Error - Ohm - unknown hyper_mode"); + }src/core/data/grid/gridlayout.hpp (4)
115-123: LGTM! Consider adding documentation for the new parameter.The addition of the
level_numberparameter to the constructor is well-implemented and maintains backward compatibility. The initialization of thelevelNumber_member is correct.Consider adding a brief comment explaining the purpose of the
level_numberparameter in the constructor's documentation.
1174-1174: LGTM! Consider explicit return type.The
levelNumber()method is correctly implemented as a const getter for thelevelNumber_member.Consider using an explicit return type for clarity:
-auto levelNumber() const { return levelNumber_; } +int levelNumber() const { return levelNumber_; }
Line range hint
1077-1126: LGTM! Consider adding documentation for the new methods.The new B to E projection methods (
BxToEx(),ByToEy(),BzToEz()) are correctly implemented as static constexpr methods delegating toGridLayoutImpl. This approach is consistent with other similar methods in the class.Consider adding brief documentation comments for these new methods, explaining their purpose and return values, similar to the existing comments for other projection methods in the class.
Line range hint
115-1519: Overall, the changes enhance the GridLayout class effectively.The additions to the
GridLayoutclass, including the newlevel_numberparameter in the constructor, thelevelNumber()getter, and the B to E projection methods, are well-implemented and consistent with the existing code style. These changes enhance the functionality of the class without introducing breaking changes.Consider the following suggestions for future improvements:
- Add comprehensive documentation for the new
level_numberconcept, explaining its purpose and how it relates to the overall grid layout system.- If the B to E projections are part of a larger feature, consider adding a comment block explaining the overall purpose and any related methods or classes that work together to implement this feature.
- Ensure that any code using the
GridLayoutclass is updated to take advantage of the newlevel_numberfunctionality where appropriate.src/core/data/grid/gridlayoutimplyee.hpp (2)
688-728: Approved: Well-implemented transformation with a minor suggestionThe
BxToExfunction correctly implements the transformation from Bx (primal dual dual) to Ex (dual primal primal) for all dimensions (1D, 2D, 3D). The use ofconstexpris appropriate for compile-time optimization.Consider using a single
constexprvariable for the weight calculation to improve readability:constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.125;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors.
788-828: Approved: Correct implementation with a minor suggestionThe
BzToEzfunction correctly implements the transformation from Bz (dual dual primal) to Ez (primal primal dual) for all dimensions (1D, 2D, 3D). The use ofconstexpris appropriate for compile-time optimization.Similar to the previous suggestion, consider using a single
constexprvariable for the weight calculation to improve readability:constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.25;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- .github/workflows/cmake_ubuntu.yml (1 hunks)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (2 hunks)
- src/amr/resources_manager/amr_utils.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/data/grid/gridlayoutimplyee.hpp (3 hunks)
- src/core/data/tensorfield/tensorfield.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pyphare/pyphare/pharein/init.py
- pyphare/pyphare/pharein/simulation.py
- src/amr/resources_manager/amr_utils.hpp
- src/core/data/tensorfield/tensorfield.hpp
🧰 Additional context used
📓 Path-based instructions (4)
src/core/data/grid/gridlayout.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/grid/gridlayoutimplyee.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/vecfield/vecfield.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 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 (6)
.github/workflows/cmake_ubuntu.yml (1)
77-77: Approved: Build type change improves debugging capabilitiesThe change from
RelWithDebInfotoDebugbuild type for SAMRAI, while adding-O3toCMAKE_CXX_FLAGS, is a good approach. This keeps asserts enabled (which are typically disabled inRelWithDebInfo) while still maintaining optimizations. This aligns with the project's preference for keeping asserts enabled during development and testing.src/core/numerics/ohm/ohm.hpp (4)
15-15: LGTM: HyperMode enum additionThe
HyperModeenum is well-defined and clearly represents the two modes of operation for the hyperresistivity calculation. This addition enhances the code's readability and type safety.
359-390: 🛠️ Refactor suggestion
⚠️ Potential issueAddress potential division by zero and consider minor optimization in
spatial_hyperresistive_The new
spatial_hyperresistive_method is well-structured and uses a lambda to reduce code duplication. However, there are two points to consider:
- Potential division by zero: In the lambda function, there's a division by
nOnE. Add a check to prevent division by zero:auto computeHR = [&](auto BxProj, auto ByProj, auto BzProj, auto nProj) { auto const BxOnE = GridLayout::project(B(Component::X), index, BxProj); auto const ByOnE = GridLayout::project(B(Component::Y), index, ByProj); auto const BzOnE = GridLayout::project(B(Component::Z), index, BzProj); auto const nOnE = GridLayout::project(n, index, nProj); auto b = std::sqrt(BxOnE * BxOnE + ByOnE * ByOnE + BzOnE * BzOnE); + if (nOnE == 0) { + return 0.0; // or throw an exception, depending on your error handling strategy + } return -nu_ * b / nOnE * lvlCoeff * layout_->laplacian(J(component), index); };
- Minor optimization: The
lvlCoeffcalculation could be moved outside the lambda since it doesn't depend on the lambda parameters:template<auto component, typename VecField, typename Field> auto spatial_hyperresistive_(VecField const& J, VecField const& B, Field const& n, MeshIndex<VecField::dimension> index) const { // TODO : https://github.com/PHAREHUB/PHARE/issues/3 auto const lvlCoeff = 1. / std::pow(4, layout_->levelNumber()); - auto computeHR = [&](auto BxProj, auto ByProj, auto BzProj, auto nProj) { + auto computeHR = [&, lvlCoeff](auto BxProj, auto ByProj, auto BzProj, auto nProj) { // ... (rest of the lambda implementation) }; // ... (rest of the method implementation) }These changes will improve the robustness and potentially the performance of the
spatial_hyperresistive_method.Likely invalid or redundant comment.
354-357: LGTM: constant_hyperresistive_ method signature updateThe
constant_hyperresistive_method signature has been correctly updated to match the changes in thehyperresistive_method, ensuring consistency in the class interface.Regarding the TODO comment, could you provide more information about the plans for addressing issue #3? This might help in understanding the future direction of this method.
✅ Verification successful
Verified: constant_hyperresistive_ method and TODO comment
The
constant_hyperresistive_method signature update aligns with thehyperresistive_method changes, ensuring consistency in the class interface.The TODO comment references issue #3, which is currently open and outlines plans to implement switches for resistivity and hyperresistivity terms. This clarifies the future direction for the method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Fetch the content of issue #3 gh issue view 3 --repo PHAREHUB/PHARELength of output: 1172
Script:
#!/bin/bash # Corrected: Fetch the content of issue #3 gh issue view 3 --repo PHAREHUB/PHARELength of output: 1044
12-12: LGTM: Removal of unused<numeric>headerThe removal of the
<numeric>header is appropriate if it's no longer being used in this file. This change helps reduce compilation time and potential conflicts.To ensure that the
<numeric>header is indeed unnecessary, please run the following script to search for anystd::functions that might be using it:✅ Verification successful
Verified: Removal of unused
<numeric>headerThe
<numeric>header is not utilized in this file, as nostd::functions from<numeric>are present. Removing it helps reduce unnecessary dependencies and improves compilation efficiency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of std:: functions from the <numeric> header rg --type cpp 'std::\w+' src/core/numerics/ohm/ohm.hppLength of output: 1172
Script:
#!/bin/bash # Search for any usage of std:: functions from the <numeric> header rg --type cpp 'std::\w+' src/core/numerics/ohm/ohm.hppLength of output: 428
src/core/data/grid/gridlayoutimplyee.hpp (1)
Line range hint
1-1183: Overall assessment: Well-implemented changes with minor improvements suggestedThe new functions
BxToEx,BzToEz, andByToEysuccessfully implement the required projections of the magnetic field (B) onto the electric field (E) for all dimensions (1D, 2D, 3D). These changes align well with the PR objectives and enhance the functionality related to hyperresistivity within the PHARE framework.Key points:
- The implementations are correct and follow a consistent pattern across functions.
- Proper use of
constexprandstaticfor compile-time optimization.- Correct handling of different dimensions and shifts.
Suggestions for improvement:
- Use a single
constexprvariable for weight calculations to improve readability.- Fix the minor issue with missing variable names in the
ByToEyfunction for the 3D case.These changes provide a solid foundation for computing the norm of the magnetic field at specific E locations, as intended by the PR.
src/core/numerics/ohm/ohm.hpp
Outdated
| @@ -281,6 +286,7 @@ class Ohm : public LayoutHolder<GridLayout> | |||
| return -gradPOnEy / nOnEy; | |||
| } | |||
| else | |||
| #include <cmath> | |||
There was a problem hiding this comment.
Move <cmath> header inclusion to the top of the file
The <cmath> header is currently included within a method body, which is incorrect and unconventional. Move this inclusion to the top of the file with the other header inclusions for better organization and to ensure it's available throughout the entire file.
Apply this change:
#ifndef PHARE_OHM_HPP
#define PHARE_OHM_HPP
+#include <cmath>
#include "core/data/grid/gridlayoutdefs.hpp"
#include "core/data/grid/gridlayout_utils.hpp"
#include "core/data/vecfield/vecfield_component.hpp"
#include "initializer/data_provider.hpp"
-// ... (later in the file)
-#include <cmath>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <cmath> | |
| #ifndef PHARE_OHM_HPP | |
| #define PHARE_OHM_HPP | |
| #include <cmath> | |
| #include "core/data/grid/gridlayoutdefs.hpp" | |
| #include "core/data/grid/gridlayout_utils.hpp" | |
| #include "core/data/vecfield/vecfield_component.hpp" | |
| #include "initializer/data_provider.hpp" |
| NO_DISCARD auto static constexpr ByToEy() | ||
| { | ||
| // By is dual primal dual | ||
| // Ey is primal dual primal | ||
| // the operation is thus dpd to pdp | ||
| auto constexpr p2dShift = primalToDual(); | ||
| auto constexpr d2pShift = dualToPrimal(); | ||
|
|
||
| if constexpr (dimension == 1) | ||
| { | ||
| constexpr WeightPoint<dimension> P1{Point<int, dimension>{0}, 0.5}; | ||
| constexpr WeightPoint<dimension> P2{Point<int, dimension>{d2pShift}, 0.5}; | ||
| return std::array<WeightPoint<dimension>, 2>{P1, P2}; | ||
| } | ||
| else if constexpr (dimension == 2) | ||
| { | ||
| constexpr WeightPoint<dimension> P1{Point<int, dimension>{0, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P2{Point<int, dimension>{d2pShift, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P3{Point<int, dimension>{0, p2dShift}, 0.25}; | ||
| constexpr WeightPoint<dimension> P4{Point<int, dimension>{d2pShift, p2dShift}, | ||
| 0.25}; | ||
| return std::array<WeightPoint<dimension>, 4>{P1, P2, P3, P4}; | ||
| } | ||
| else if constexpr (dimension == 3) | ||
| { | ||
| constexpr WeightPoint<dimension> P1{Point<int, dimension>{0, 0, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P2{Point<int, dimension>{d2pShift, 0, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P3{Point<int, dimension>{0, p2dShift, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P4{Point<int, dimension>{d2pShift, p2dShift, 0}, | ||
| 0.25}; | ||
| constexpr WeightPoint<dimension> P5{Point<int, dimension>{0, 0, d2pShift}, 0.25}; | ||
| constexpr WeightPoint<dimension> P6{Point<int, dimension>{d2pShift, 0, d2pShift}, | ||
| 0.25}; | ||
| constexpr WeightPoint<dimension> P7{Point<int, dimension>{0, p2dShift, d2pShift}, | ||
| 0.25}; | ||
| constexpr WeightPoint<dimension> P8{ | ||
| Point<int, dimension>{d2pShift, p2dShift, d2pShift}, 0.25}; | ||
| return std::array<WeightPoint<dimension>, 8>{P1, P2, P3, P4, P5, P6, P7, P8}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Approved with suggestions: Correct implementation with minor issues
The ByToEy function is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from By (dual primal dual) to Ey (primal dual primal). The use of constexpr and static is appropriate for compile-time optimization.
In the 3D case, there's a minor issue where P6, P7, and P8 are missing variable names. This should be corrected to ensure proper compilation. Apply the following diff to fix this issue:
- constexpr WeightPoint<dimension>{Point<int, dimension>{d2pShift, 0, d2pShift},
- 0.25};
- constexpr WeightPoint<dimension>{Point<int, dimension>{0, p2dShift, d2pShift},
- 0.25};
- constexpr WeightPoint<dimension>{
- Point<int, dimension>{d2pShift, p2dShift, d2pShift}, 0.25};
+ constexpr WeightPoint<dimension> P6{Point<int, dimension>{d2pShift, 0, d2pShift},
+ 0.25};
+ constexpr WeightPoint<dimension> P7{Point<int, dimension>{0, p2dShift, d2pShift},
+ 0.25};
+ constexpr WeightPoint<dimension> P8{
+ Point<int, dimension>{d2pShift, p2dShift, d2pShift}, 0.25};For consistency with the previous functions, consider using a single constexpr variable for the weight calculation:
constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.25;Then use this weight variable instead of hard-coded values in the WeightPoint constructors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/core/numerics/ohm/ohm.hpp (3)
27-29: LGTM:hyper_modeinitialization addedThe initialization of
hyper_modein the constructor is well-implemented. However, consider using a case-insensitive comparison for better robustness:- , hyper_mode{cppdict::get_value(dict, "hyper_mode", std::string{"constant"}) == "constant" + , hyper_mode{std::tolower(cppdict::get_value(dict, "hyper_mode", std::string{"constant"})[0]) == 'c' ? HyperMode::constant : HyperMode::spatial}This change would allow for variations like "Constant" or "CONSTANT" to be correctly interpreted.
353-356: LGTM:constant_hyperresistive_method signature updatedThe method signature has been correctly updated to match the changes in the
hyperresistive_method.The TODO comment referencing issue #3 (#3) indicates that there are pending changes. Consider creating a separate task to track this TODO item and ensure it's addressed in future updates.
Would you like me to create a GitHub issue to track this TODO item?
358-389: LGTM: Newspatial_hyperresistive_method added with suggestionsThe new
spatial_hyperresistive_method is well-implemented. Here are some suggestions for improvement:
- Add a comment explaining the purpose of the
lvlCoeffcalculation.- Add a check to prevent division by zero when
nOnEis zero:auto computeHR = [&](auto BxProj, auto ByProj, auto BzProj, auto nProj) { auto const BxOnE = GridLayout::project(B(Component::X), index, BxProj); auto const ByOnE = GridLayout::project(B(Component::Y), index, ByProj); auto const BzOnE = GridLayout::project(B(Component::Z), index, BzProj); auto const nOnE = GridLayout::project(n, index, nProj); auto b = std::sqrt(BxOnE * BxOnE + ByOnE * ByOnE + BzOnE * BzOnE); + if (nOnE == 0) { + return 0.0; // or throw an exception, depending on your error handling strategy + } return -nu_ * b / nOnE * lvlCoeff * layout_->laplacian(J(component), index); };
- Consider creating a separate task to track the TODO item referencing issue implement switches for resistivity and hyperresistivity terms #3.
Would you like me to create a GitHub issue to track the TODO item mentioned in the comment?
src/core/data/grid/gridlayout.hpp (2)
115-123: LGTM! Consider adding documentation for the new parameter.The addition of the
level_numberparameter to the constructor is a good improvement for supporting multi-level simulations. The default value of 0 ensures backward compatibility.Consider adding a brief comment explaining the purpose of the
level_numberparameter in the constructor's documentation.
Line range hint
1077-1126: LGTM! Consider adding brief comments for consistency.The addition of
BxToEx(),ByToEy(), andBzToEz()methods is a good improvement for magnetic to electric field component conversions. They are correctly implemented as static constexpr methods and delegate to theGridLayoutImpl.For consistency with other similar methods in this class (e.g.,
momentsToEx()), consider adding brief comments explaining the purpose of these new methods. For example:/** * @brief BxToEx returns the indexes and associated coef to compute the linear * interpolation necessary to project Bx onto Ex. */ NO_DISCARD auto static constexpr BxToEx() { return GridLayoutImpl::BxToEx(); }src/core/data/grid/gridlayoutimplyee.hpp (3)
688-728: Approved: Well-implemented transformation functionThe
BxToExfunction is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from Bx (primal dual dual) to Ex (dual primal primal). The use ofconstexprandstaticis appropriate for compile-time optimization.Consider adding a brief comment explaining the meaning of the weight values (0.5, 0.25, 0.125) for better code readability.
788-828: Approved: Correct implementation with a minor suggestionThe
BzToEzfunction is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from Bz (dual dual primal) to Ez (primal primal dual). The use ofconstexprandstaticis appropriate for compile-time optimization.For consistency with the
BxToExfunction, consider using a singleconstexprvariable for the weight calculation:constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.25;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors.
923-962: Approved: Correct implementation with consistency suggestionThe
ByToEyfunction is correctly implemented for all dimensions (1D, 2D, 3D) and properly handles the transformation from By (dual primal dual) to Ey (primal dual primal). The use ofconstexprandstaticis appropriate for compile-time optimization.For consistency with the previous functions, consider using a single
constexprvariable for the weight calculation:constexpr double weight = (dimension == 1) ? 0.5 : (dimension == 2) ? 0.25 : 0.25;Then use this
weightvariable instead of hard-coded values in theWeightPointconstructors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- .github/workflows/cmake_ubuntu.yml (1 hunks)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (3 hunks)
- src/amr/resources_manager/amr_utils.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/data/grid/gridlayoutimplyee.hpp (3 hunks)
- src/core/data/tensorfield/tensorfield.hpp (1 hunks)
- src/core/data/vecfield/vecfield.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/cmake_ubuntu.yml
- pyphare/pyphare/pharein/simulation.py
- src/amr/resources_manager/amr_utils.hpp
- src/core/data/tensorfield/tensorfield.hpp
- src/core/data/vecfield/vecfield.hpp
🧰 Additional context used
📓 Path-based instructions (3)
src/core/data/grid/gridlayout.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/grid/gridlayoutimplyee.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ohm/ohm.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (5)
src/core/numerics/ohm/ohm.hpp (2)
15-16: LGTM: New enumHyperModeaddedThe addition of the
HyperModeenum is well-implemented. It provides a clear and type-safe way to specify the hyperresistivity mode.
339-349: LGTM:hyperresistive_method updatedThe changes to the
hyperresistive_method are well-implemented, providing flexibility based on thehyper_mode. The error handling for an unknown mode is appropriate.However, the TODO comment that was present in the previous version has been removed. Please verify if the issue referenced in that comment has been addressed or if it should be maintained.
src/core/data/grid/gridlayout.hpp (2)
1174-1174: LGTM! Good addition of the accessor method.The
levelNumber()method is a well-implemented accessor for the newlevelNumber_member. It correctly provides read-only access and is marked asconst.
Line range hint
1-1519: Overall, good improvements to support multi-level simulations and field conversions.The changes to the
GridLayoutclass, including the addition of thelevel_numberparameter,levelNumber()accessor, and B to E field conversion methods, enhance the functionality for multi-level simulations and improve the flexibility of field component manipulations. These additions are well-implemented and maintain good coding practices.Consider addressing the minor documentation suggestions to further improve code clarity and maintainability.
pyphare/pyphare/pharein/__init__.py (1)
239-242: Additions of Ohm's Law parameters are correctly implementedThe code correctly adds
resistivity,hyper_resistivity, andhyper_modeto the simulation configuration within thepopulateDictfunction. This enhancement allows for better configurability of the simulation's Ohm's Law parameters, supporting spatially variable hyperresistivity as described in PR #893.
| { // TODO : https://github.com/PHAREHUB/PHARE/issues/3 | ||
| auto const lvlCoeff = 1. / std::pow(4, layout_->levelNumber()); | ||
|
|
||
| auto computeHR = [&](auto BxProj, auto ByProj, auto BzProj, auto nProj) { |
Check notice
Code scanning / CodeQL
Large object passed by value Note
| { // TODO : https://github.com/PHAREHUB/PHARE/issues/3 | ||
| auto const lvlCoeff = 1. / std::pow(4, layout_->levelNumber()); | ||
|
|
||
| auto computeHR = [&](auto BxProj, auto ByProj, auto BzProj, auto nProj) { |
Check notice
Code scanning / CodeQL
Large object passed by value Note
| { // TODO : https://github.com/PHAREHUB/PHARE/issues/3 | ||
| auto const lvlCoeff = 1. / std::pow(4, layout_->levelNumber()); | ||
|
|
||
| auto computeHR = [&](auto BxProj, auto ByProj, auto BzProj, auto nProj) { |
Check notice
Code scanning / CodeQL
Large object passed by value Note
implements #880
Summary by CodeRabbit
New Features
hyperresistive_method in theOhmclass for improved physical modeling of electric and magnetic field interactions.hyper_modeconfiguration option for simulations, allowing for more nuanced resistivity calculations.GridLayoutfunctionality by including a level number parameter for improved grid management.Bug Fixes
TensorFieldclass without altering functionality.Chores