Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications across several files, primarily focusing on enhancing simulation parameters and functionalities. Key updates include the expansion of the 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: 6
🧹 Outside diff range and nitpick comments (12)
src/amr/solvers/solver_ppc.hpp (2)
155-170: LGTM! Consider a minor optimization.The new
add_tomethod is well-implemented and efficiently handles both new and existing keys in the map. The use ofreservebeforereplace_fromis a good practice for performance.Consider using
emplaceinstead ofemplacefollowed by assignment for a slight performance improvement:- if (!map.count(key)) - map.emplace(key, empty); - else - map.at(key) = empty; + auto [it, inserted] = map.try_emplace(key, empty); + if (!inserted) + it->second = empty;This change reduces the number of potential allocations and moves.
225-227: LGTM! Consider using string concatenation for key construction.The changes in the
saveState_method improve code consistency and readability by using the newadd_tomethod. The key construction ensures unique keys for each patch and population combination.Consider using string concatenation instead of
stringstreamfor key construction, as it's simpler and potentially more efficient for this use case:- std::string const key = ss.str() + "_" + pop.name(); + std::string const key = std::to_string(state.patch->getGlobalId()) + "_" + pop.name();This change eliminates the need for the
stringstreamobject and simplifies the key construction.pyphare/pyphare/pharein/simulation.py (2)
650-650: Consider adding validation for thehyper_modeparameter.The
hyper_modeparameter has been added to the list of accepted keywords with a default value of "constant". However, there's no validation to ensure that only valid modes are accepted. Consider adding a check to validate thehyper_modevalue to prevent potential issues with invalid modes.You could add a function similar to
check_clustering()to validate thehyper_mode:def check_hyper_mode(**kwargs): valid_modes = ["constant", "other_valid_mode"] # Add all valid modes here hyper_mode = kwargs.get("hyper_mode", "constant") if hyper_mode not in valid_modes: raise ValueError(f"Error: hyper_mode '{hyper_mode}' is not supported. Valid modes are {valid_modes}") return hyper_modeThen call this function in the
checkerdecorator:kwargs["hyper_mode"] = check_hyper_mode(**kwargs)
723-723: LGTM! Consider adding documentation for the newhyper_modeparameter.The addition of the
hyper_modeparameter in the__init__method is consistent with its inclusion in thecheckerfunction. The default value is maintained, which is good for consistency.To improve the code's documentation, consider adding a description of the
hyper_modeparameter in the class docstring. For example:class Simulation(object): """ ... :Keyword Arguments: ... * *hyper_mode* (``str``)-- Mode for hyper-resistivity calculation (default="constant"). Valid values are ["constant", ...] # Add other valid modes """src/core/numerics/ohm/ohm.hpp (3)
352-353: Handle unexpectedhyper_modevalues explicitlyIn the
hyperresistive_method, theelseblock returns0.0to satisfy the compiler's requirement for a return statement. This could mask unintended states ifhyper_modehas an unexpected value.For better robustness and maintainability, consider handling unexpected
hyper_modevalues explicitly by throwing an exception or using an assert to catch invalid states during development. For example:else // should not happen but otherwise -Wreturn-type fails with Werror - return 0.; +#ifdef NDEBUG + throw std::runtime_error("Invalid hyper_mode in hyperresistive_ method"); +#else + assert(false && "Invalid hyper_mode in hyperresistive_ method"); +#endif
360-362: Address the TODO comment for hyper-resistivity implementationThe
constant_hyperresistive_method contains a TODO comment referring to issue #3. This indicates that the implementation may be incomplete or requires further attention.Do you need assistance in addressing this TODO? If so, I can help implement the required changes or provide guidance. Let me know if you'd like me to open a new GitHub issue to track this task.
367-389: Address the TODO comment inspatial_hyperresistive_methodThe
spatial_hyperresistive_method also contains a TODO comment referring to issue #3. This suggests that further implementation or refinement is needed for spatial hyper-resistivity.Would you like help in completing the implementation of this method? I can provide assistance or open a GitHub issue to track this work.
src/core/data/grid/gridlayout.hpp (5)
1174-1174: MarklevelNumber()withNO_DISCARDto prevent ignored return valuesTo avoid situations where the return value of
levelNumber()is unintentionally ignored, consider marking the function with theNO_DISCARDattribute.Apply this diff:
-auto levelNumber() const { return levelNumber_; } +NO_DISCARD auto levelNumber() const { return levelNumber_; }
1519-1519: Remove redundant default initialization oflevelNumber_The member variable
levelNumber_is default-initialized to0in its declaration and also set in the constructor's initializer list. The default initialization is redundant and can be removed to avoid confusion.Apply this diff:
-int levelNumber_ = 0; +int levelNumber_;
1077-1077: Add missing documentation for new projection methodsThe newly added methods
BxToEx(),BxToEy(),ByToEy(), andBzToEz()lack documentation comments. For consistency and better maintainability, please add Doxygen-style comments describing the purpose and usage of each method, similar to existing methods in the class.Apply this diff to add comments:
+/** + * @brief BxToEx returns indexes and coefficients to interpolate Bx onto Ex. + */ NO_DISCARD auto static constexpr BxToEx() { return GridLayoutImpl::BxToEx(); } +/** + * @brief BxToEy returns indexes and coefficients to interpolate Bx onto Ey. + */ NO_DISCARD auto static constexpr BxToEy() { return GridLayoutImpl::BxToEy(); } +/** + * @brief ByToEy returns indexes and coefficients to interpolate By onto Ey. + */ NO_DISCARD auto static constexpr ByToEy() { return GridLayoutImpl::ByToEy(); } +/** + * @brief BzToEz returns indexes and coefficients to interpolate Bz onto Ez. + */ NO_DISCARD auto static constexpr BzToEz() { return GridLayoutImpl::BzToEz(); }Also applies to: 1093-1093, 1101-1101, 1125-1125
1077-1077: Adjust order of function specifiers for consistencyThe order of specifiers in the methods
BxToEx(),BxToEy(),ByToEy(), andBzToEz()is unconventional. In C++, the recommended order isstatic constexpr, followed by the return type. Adjusting the order improves readability and maintains consistency with the rest of the codebase.Apply this diff to reorder the specifiers:
-NO_DISCARD auto static constexpr BxToEx() { return GridLayoutImpl::BxToEx(); } +NO_DISCARD static constexpr auto BxToEx() { return GridLayoutImpl::BxToEx(); } -NO_DISCARD auto static constexpr BxToEy() { return GridLayoutImpl::BxToEy(); } +NO_DISCARD static constexpr auto BxToEy() { return GridLayoutImpl::BxToEy(); } -NO_DISCARD auto static constexpr ByToEy() { return GridLayoutImpl::ByToEy(); } +NO_DISCARD static constexpr auto ByToEy() { return GridLayoutImpl::ByToEy(); } -NO_DISCARD auto static constexpr BzToEz() { return GridLayoutImpl::BzToEz(); } +NO_DISCARD static constexpr auto BzToEz() { return GridLayoutImpl::BzToEz(); }Also applies to: 1093-1093, 1101-1101, 1125-1125
1174-1174: Consistency in accessor method namingThe method
levelNumber()provides access to the member variablelevelNumber_. Ensure that the naming conventions for accessor methods are consistent throughout the codebase. If other accessor methods usegetor another prefix, consider renaming this method to match.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (2 hunks)
- src/amr/resources_manager/amr_utils.hpp (1 hunks)
- src/amr/solvers/solver_ppc.hpp (2 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.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 due to trivial changes (2)
- src/core/data/tensorfield/tensorfield.hpp
- src/core/data/vecfield/vecfield.hpp
🧰 Additional context used
📓 Path-based instructions (6)
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/amr/solvers/solver_ppc.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.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.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 (7)
src/core/numerics/ohm/ohm.hpp (1)
293-293:⚠️ Potential issueIncorrect placement of
#include <cmath>inside functionThe
#include <cmath>directive is placed inside thepressure_function at line 293. Including headers within function bodies is not standard practice and may lead to compilation issues or unexpected behavior.Move the
#include <cmath>directive to the top of the file with the other#includestatements. Apply this diff to fix the issue:Place
#include <cmath>with the other includes:Likely invalid or redundant comment.
pyphare/pyphare/pharein/__init__.py (3)
240-242: Verify initialization of Ohm's law parameters inSimulationThe parameters
simulation.resistivity,simulation.hyper_resistivity, andsimulation.hyper_modeare added to the configuration. Ensure that these attributes are initialized in theSimulationclass with appropriate default values to avoidAttributeErrorif they are not provided by the user.Use the following script to confirm that these attributes are defined in the
Simulationclass:#!/bin/bash # Description: Check if 'resistivity', 'hyper_resistivity', and 'hyper_mode' are initialized in 'Simulation' class. # Search for the '__init__' method in 'Simulation' where these attributes should be set. ast-grep --lang python --pattern $'class Simulation($$$_): $$$ def __init__($$$_): $$$ self.resistivity = $_ self.hyper_resistivity = $_ self.hyper_mode = $_ $$$ $$$'
242-242: Confirm consistency of configuration paths for Ohm's law parametersEnsure that the configuration paths for
resistivity,hyper_resistivity, andhyper_modeunder"simulation/algo/ohm/"are consistent throughout the codebase and align with how other algorithms and parameters are structured.Run the following script to check for other usages of the
"simulation/algo/ohm/"path:#!/bin/bash # Description: Search for occurrences of the Ohm's law configuration path to verify consistency. # Find all instances where the Ohm's law path is used. rg --type py '("|\')simulation/algo/ohm/.*("|\')'
239-239: Ensuresimulation.particle_pusheris properly initializedThe line adds
simulation.particle_pusherto the configuration. Please verify thatsimulation.particle_pusheris set and valid before this point to prevent potentialAttributeErrorexceptions if it isNoneor not defined.To check where
simulation.particle_pusheris set and ensure it has a default value, you can run the following script:src/core/data/grid/gridlayout.hpp (1)
123-123: Initialize member variable in initializer listThe member variable
levelNumber_is correctly initialized in the constructor's member initializer list, ensuring it holds the intended value upon object creation.src/core/data/grid/gridlayoutimplyee.hpp (2)
788-828: Apply the refactoring suggestion toBzToEz()Similar to
BxToEx(), theBzToEz()method has repetitive code blocks for each dimension. Refactoring this method to reduce code duplication will improve code clarity and maintainability.
923-962: Apply the refactoring suggestion toByToEy()The
ByToEy()method also presents repetitive structures for handling different dimensions. Using the same refactoring approach as suggested forBxToEx()can enhance the codebase by reducing redundancy.
| 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)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Approved: Changes align with PR objectives and improve formula consistency.
The modifications to the field_diff lambda function address the PR objective of incorporating absolute values in the tagging formula. This should help in refining the two current layers uniformly across different thresholds.
A minor optimization suggestion:
Consider factoring out the common std::abs() call in the denominator of the second tuple element:
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));
+ / (1 + std::abs(F(ix, iy + 1) - F(ix, iy))));This change makes the second tuple element consistent with the first one and potentially improves readability.
📝 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.
| 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(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)) | |
| / (1 + std::abs(F(ix, iy + 1) - F(ix, iy)))); |
| auto lvlNbr = patch.getPatchLevelNumber(); | ||
| return GridLayoutT{dl, nbrCell, origin, amr::Box<int, dimension>{domain}, lvlNbr}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving backwards compatibility and documentation
The addition of the patch level number (lvlNbr) to the GridLayoutT constructor is a valuable change that can enhance level-specific operations and debugging. However, this modification has some implications:
- The function signature change may break existing code that uses
layoutFromPatch. - The function's documentation hasn't been updated to reflect this new parameter.
To address these issues:
-
Consider using a default parameter to maintain backwards compatibility:
NO_DISCARD GridLayoutT layoutFromPatch(SAMRAI::hier::Patch const& patch, int lvlNbr = -1)
If
lvlNbris not provided, you can still obtain it frompatch.getPatchLevelNumber(). -
Update the function documentation to describe the new
lvlNbrparameter and its purpose.
Here's a suggested implementation with improved documentation:
/**
* @brief Create a GridLayoutT from a SAMRAI::hier::Patch
* @param patch The SAMRAI patch to create the layout from
* @param lvlNbr The patch level number (optional, default: -1)
* @return A GridLayoutT instance representing the patch
*/
template<typename GridLayoutT>
NO_DISCARD GridLayoutT layoutFromPatch(SAMRAI::hier::Patch const& patch, int lvlNbr = -1)
{
// ... (existing code)
if (lvlNbr == -1)
{
lvlNbr = patch.getPatchLevelNumber();
}
return GridLayoutT{dl, nbrCell, origin, amr::Box<int, dimension>{domain}, lvlNbr};
}This approach maintains backwards compatibility while allowing explicit specification of the level number when needed.
| auto const nOnE = GridLayout::project(n, index, nProj); | ||
| auto b = std::sqrt(BxOnE * BxOnE + ByOnE * ByOnE + BzOnE * BzOnE); | ||
| return -nu_ * b / nOnE * lvlCoeff * layout_->laplacian(J(component), index); |
There was a problem hiding this comment.
Potential division by zero in spatial_hyperresistive_ method
In the spatial_hyperresistive_ method, the calculation involves dividing by nOnE, which could be zero if the electron density is zero at a given point. This may lead to division by zero errors during runtime.
Consider adding a check to ensure that nOnE is not zero before performing the division, or handle zero-density cases appropriately. For example:
auto const nOnE = GridLayout::project(n, index, nProj);
+if (nOnE == 0.0) {
+ // Handle zero-density case to prevent division by zero
+ return 0.0;
+}
auto b = std::sqrt(BxOnE * BxOnE + ByOnE * ByOnE + BzOnE * BzOnE);
return -nu_ * b / nOnE * lvlCoeff * layout_->laplacian(J(component), index);Committable suggestion was skipped due to low confidence.
| constexpr static std::array<int, 2> nextIndexTable_{{nextPrimal_(), nextDual_()}}; | ||
| constexpr static std::array<int, 2> prevIndexTable_{{prevPrimal_(), prevDual_()}}; | ||
|
|
||
| int levelNumber_ = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using std::size_t for levelNumber_
Since levelNumber_ represents a non-negative level index, using an unsigned type like std::size_t can prevent negative values and improve type safety.
Apply this diff:
-int levelNumber_;
+std::size_t levelNumber_;📝 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.
| int levelNumber_ = 0; | |
| std::size_t levelNumber_ = 0; |
| std::array<std::uint32_t, dimension> const& nbrCells, | ||
| Point<double, dimension> const& origin, | ||
| Box<int, dimension> AMRBox = Box<int, dimension>{}) | ||
| Box<int, dimension> AMRBox = Box<int, dimension>{}, int level_number = 0) |
There was a problem hiding this comment.
Ensure consistent parameter type for level_number
The new constructor parameter int level_number = 0 introduces the level_number as an int. Since level_number represents a non-negative value (level index), consider using std::size_t or unsigned int for consistency and to prevent negative values.
Apply this diff to change the parameter type:
-Box<int, dimension> AMRBox = Box<int, dimension>{}, int level_number = 0)
+Box<int, dimension> AMRBox = Box<int, dimension>{}, std::size_t level_number = 0)📝 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.
| Box<int, dimension> AMRBox = Box<int, dimension>{}, int level_number = 0) | |
| Box<int, dimension> AMRBox = Box<int, dimension>{}, std::size_t level_number = 0) |
| NO_DISCARD auto static constexpr BxToEx() | ||
| { | ||
| // Bx is primal dual dual | ||
| // Ex is dual primal primal | ||
| // operation is pdd to dpp | ||
| [[maybe_unused]] auto constexpr p2dShift = primalToDual(); | ||
| [[maybe_unused]] 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>{p2dShift}, 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>{0, d2pShift}, 0.25}; | ||
| constexpr WeightPoint<dimension> P3{Point<int, dimension>{p2dShift, 0}, 0.25}; | ||
| constexpr WeightPoint<dimension> P4{Point<int, dimension>{p2dShift, d2pShift}, | ||
| 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.125}; | ||
| constexpr WeightPoint<dimension> P2{Point<int, dimension>{0, d2pShift, 0}, 0.125}; | ||
| constexpr WeightPoint<dimension> P3{Point<int, dimension>{p2dShift, 0, 0}, 0.125}; | ||
| constexpr WeightPoint<dimension> P4{Point<int, dimension>{p2dShift, d2pShift, 0}, | ||
| 0.125}; | ||
|
|
||
| constexpr WeightPoint<dimension> P5{Point<int, dimension>{0, 0, d2pShift}, 0.125}; | ||
| constexpr WeightPoint<dimension> P6{Point<int, dimension>{0, d2pShift, d2pShift}, | ||
| 0.125}; | ||
| constexpr WeightPoint<dimension> P7{Point<int, dimension>{p2dShift, 0, d2pShift}, | ||
| 0.125}; | ||
| constexpr WeightPoint<dimension> P8{ | ||
| Point<int, dimension>{p2dShift, d2pShift, d2pShift}, 0.125}; | ||
| return std::array<WeightPoint<dimension>, 8>{P1, P2, P3, P4, P5, P6, P7, P8}; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring BxToEx() to reduce code duplication
The BxToEx() method contains similar code blocks for different dimensions (dimension == 1, dimension == 2, and dimension == 3). This can lead to code duplication and increase maintenance effort. To enhance maintainability and readability, consider refactoring by creating a helper function or using templates to handle the common logic across dimensions.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // template<typename VecField, typename... Index> | ||
| // NO_DISCARD auto norm(VecField const& vf, Index... index) | ||
| // { | ||
| // using Type = typename VecField::value_type; | ||
| // std::sqrt(std::accumulate(std::begin(vf.components()), std::end(vf.components()), | ||
| // Type{0}, | ||
| // [&](auto acc, auto const& c) { | ||
| // auto const v = c(index...); | ||
| // return acc(index...) + v * v; | ||
| // })); | ||
| // } |
Check notice
Code scanning / CodeQL
Commented-out code
2 participants
is on top of #893
the PR only concerns the diff in default hybrid tagging and adds absolute values to the tagging formula.
Or alternatively, only the last commit ("tagging better")
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
New Features
hyper_modefor improved validation of simulation parameters.Bug Fixes
Refactor