hierarchy views #787
hierarchy views #787
Conversation
WalkthroughThe codebase has undergone a major refactoring, transitioning from the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 28
Configuration used: .coderabbit.yaml
Files selected for processing (57)
- res/cmake/dep.cmake (1 hunks)
- res/cmake/test.cmake (1 hunks)
- src/amr/data/field/field_data.hpp (2 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3 hunks)
- src/amr/multiphysics_integrator.hpp (5 hunks)
- src/amr/physical_models/hybrid_model.hpp (2 hunks)
- src/amr/resources_manager/field_resource.hpp (1 hunks)
- src/amr/resources_manager/resources_manager.hpp (1 hunks)
- src/amr/solvers/solver.hpp (5 hunks)
- src/amr/solvers/solver_mhd.hpp (2 hunks)
- src/amr/solvers/solver_ppc.hpp (8 hunks)
- src/amr/solvers/solver_ppc_model_view.hpp (1 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (1 hunks)
- src/amr/tagging/hybrid_tagger.hpp (1 hunks)
- src/core/data/electrons/electrons.hpp (9 hunks)
- src/core/data/field/field.hpp (1 hunks)
- src/core/data/grid/grid.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/data/ions/ion_population/ion_population.hpp (1 hunks)
- src/core/data/ions/ions.hpp (2 hunks)
- src/core/data/ndarray/ndarray_vector.hpp (5 hunks)
- src/core/data/vecfield/vecfield.hpp (4 hunks)
- src/core/models/hybrid_state.hpp (1 hunks)
- src/core/numerics/ampere/ampere.hpp (1 hunks)
- src/core/numerics/faraday/faraday.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (1 hunks)
- src/core/utilities/box/box.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/diagnostic/detail/types/fluid.hpp (1 hunks)
- src/phare_core.hpp (3 hunks)
- src/simulator/phare_types.hpp (1 hunks)
- tests/amr/data/field/coarsening/test_linear_coarsen.hpp (3 hunks)
- tests/amr/data/field/copy_pack/field_data_test_param.hpp (2 hunks)
- tests/amr/data/field/geometry/test_field_geometry.cpp (2 hunks)
- tests/amr/data/field/refine/test_field_refine.cpp (1 hunks)
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (2 hunks)
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.hpp (1 hunks)
- tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (2 hunks)
- tests/amr/data/field/variable/test_field_variable.cpp (2 hunks)
- tests/amr/models/test_models.cpp (1 hunks)
- tests/amr/resources_manager/CMakeLists.txt (1 hunks)
- tests/amr/resources_manager/test_resources_manager.cpp (2 hunks)
- tests/amr/resources_manager/test_resources_manager_basic_hierarchy.hpp (1 hunks)
- tests/amr/tagging/test_tagging.cpp (1 hunks)
- tests/core/data/electrons/test_electrons.cpp (9 hunks)
- tests/core/data/field/test_field.hpp (2 hunks)
- tests/core/data/grid/CMakeLists.txt (1 hunks)
- tests/core/data/grid/test_grid.cpp (9 hunks)
- tests/core/data/grid/test_grid.hpp (1 hunks)
- tests/core/data/gridlayout/gridlayout_base_params.hpp (1 hunks)
- tests/core/data/gridlayout/gridlayout_deriv.hpp (4 hunks)
- tests/core/data/gridlayout/gridlayout_laplacian.hpp (4 hunks)
- tests/core/data/ion_population/test_ion_population.cpp (1 hunks)
- tests/core/data/ions/test_ions.cpp (2 hunks)
- tests/core/data/vecfield/test_main.cpp (5 hunks)
- tests/core/numerics/ampere/test_main.cpp (6 hunks)
- tests/core/numerics/faraday/test_main.cpp (6 hunks)
Files not processed due to max files limit (4)
- tests/core/numerics/interpolator/test_main.cpp
- tests/core/numerics/ion_updater/test_updater.cpp
- tests/core/numerics/ohm/test_main.cpp
- tests/simulator/test_restarts.py
Files skipped from review due to trivial changes (9)
- res/cmake/dep.cmake
- src/amr/tagging/default_hybrid_tagger_strategy.hpp
- src/core/data/ions/ion_population/ion_population.hpp
- src/core/data/ions/ions.hpp
- src/core/utilities/box/box.hpp
- tests/amr/resources_manager/CMakeLists.txt
- tests/core/data/grid/CMakeLists.txt
- tests/core/data/gridlayout/gridlayout_deriv.hpp
- tests/core/data/ion_population/test_ion_population.cpp
Additional comments: 169
res/cmake/test.cmake (1)
- 8-14: The summary mentions that "tests/core/data/field" has been replaced with "tests/core/data/grid", but the hunk does not show the removal of "tests/core/data/field". Please verify if the summary is accurate or if the removal was omitted.
src/amr/data/field/field_data.hpp (2)
89-91: The assertion ensures that
field.vector()is not empty before attempting to populate it from the restart database. Verify that the size check is indeed intended to be non-zero and not a specific expected size. Additionally, confirm that the size of the array being read from the database matches the size offield.vector()to prevent buffer overflows or underflows.314-314: The use of
auto*in thegetPointermethod simplifies the code and ensures that the correct pointer type is returned even if the type offieldchanges. This change is consistent with modern C++ practices.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
49-88: The changes to the
FieldUserstruct to useGridTand the addition of thecopyDatamethod are consistent with the summary and appear to be correctly implemented.95-125: The changes to the
HybridHybridMessengerStrategyclass to useGridTinstead ofFieldTare consistent with the summary and appear to be correctly implemented.1005-1009: The changes to the
NiOldUser_member to useFieldUser<GridT>are consistent with the summary and appear to be correctly implemented.src/amr/multiphysics_integrator.hpp (5)
344-351: The logic to either push a new model view or update an existing one based on the
levelNumberis correctly implemented.365-374: The loop to register levels with the messenger and create model views for each level is correctly implemented.
498-502: The
advanceLevelmethod call has been correctly updated to includegetModelView_(iLevel)as an additional argument.579-579: The new member variable
model_views_has been correctly added to the class.807-807: The new private method
getModelView_has been correctly added to retrieve the model view based on the level.src/amr/physical_models/hybrid_model.hpp (4)
4-7: The addition of
#include "core/data/grid/grid.hpp"aligns with the refactoring to useGridtypes.37-38: The introduction of the
grid_typealias is consistent with the refactoring to useGridtypes.45-45: The relocation of
model_nameis consistent with the summary and does not affect functionality.33-41: The changes to type definitions such as
gridlayout_type,electromag_type,vecfield_type,field_type,grid_type, andresources_manager_typeare consistent with the refactoring to useGridtypes.src/amr/resources_manager/field_resource.hpp (1)
- 19-19: The
internal_type_ptrstill usesResourcesUser::field_type. Please verify if this is intentional or if it should also be updated toResourcesUser::grid_typeto maintain consistency with the other type changes.src/amr/resources_manager/resources_manager.hpp (2)
574-574: The
ResourcesUserparameter is commented out, which contradicts the summary stating that it is now used in the method. Please verify if the parameter should be uncommented or if the summary needs to be updated.575-575: The use of
constfor theallocateTimeparameter is a good practice to ensure it is not modified within the method.src/amr/solvers/solver.hpp (6)
19-25: The
ISolverModelViewclass has been added as an interface, which currently only includes a virtual destructor. Ensure that the implementation of this interface in derived classes is planned and that it will be utilized effectively in the codebase.47-49: The addition of type aliases
patch_t,level_t, andhierarchy_tin theISolverclass is a good practice for code readability and maintainability, as it abstracts the underlying types and makes future changes easier.89-93: The
advanceLevelmethod now includes an additional parameterISolverModelView& view. Ensure that all implementations of this method are updated accordingly and that theviewparameter is being used effectively within the method's logic.103-104: The
allocatemethod now uses the type aliaspatch_tinstead of the specificSAMRAI::hier::Patchtype. This change increases the flexibility of the code by decoupling it from a specific implementation and should be reflected in all derived classes and usages.112-113: The new virtual method
make_viewhas been added to create a view for the solver model. Ensure that the implementations of this method in derived classes provide meaningful and correct views of the solver model, and that these views are utilized where necessary.116-117: The constructor of
ISolveris now protected, which is a good practice as it prevents direct instantiation of the interface and ensures that only derived classes can be instantiated. The constructor initializessolverName, which is a clean way to set up the name for each solver instance.src/amr/solvers/solver_mhd.hpp (1)
- 38-60: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [14-59]
The changes to the
SolverMHDclass, including the addition of type aliases and modifications to function signatures, align with the summary and appear to be correctly implemented.src/amr/solvers/solver_ppc.hpp (4)
49-49: The addition of the type alias
HybridModelView_tis confirmed and aligns with the summary.97-99: The new function
make_viewis added as described in the summary and correctly returns aHybridModelView_tobject.106-124: The methods
predictor1_,predictor2_,corrector_,average_, andmoveIons_now correctly take an additional parameterHybridModelView_t& viewas mentioned in the summary.93-95: The function signature for
advanceLevelhas been updated to include the new parameterISolverModelView& viewas described in the summary.src/amr/solvers/solver_ppc_model_view.hpp (2)
38-40: The use of
dynamic_castin theregridfunction could lead to astd::bad_castexception if the cast is not valid. It's important to ensure that the function is only called with compatible types or to handle the potential exception.94-102: The
assertstatements in the iterator's dereference operators are used to ensure the validity of the current state. While this is good for debugging, it's important to note that asserts are not usually present in release builds, so they should not be relied upon for program logic.src/amr/tagging/hybrid_tagger.hpp (2)
90-93: The summary states that the second
int*argument has been removed from theNdArrayViewtemplate arguments, but the code still shows two template arguments being used. Please verify if the summary is accurate or if the code needs to be updated to reflect the intended changes.94-96: The code within the
if constexprblock is specific to 2D cases. If the application is intended to support 1D or 3D as well, corresponding code blocks should be added to handle those dimensions.src/core/data/electrons/electrons.hpp (12)
139-139: The summary states that
ions_was changed from a reference to an object, but the code still shows it as a reference. Please verify this discrepancy.140-141: The summary states that
J_andVe_were changed from references to objects, but the code still shows them as references. Please verify this discrepancy.157-160: The change to the constructor of
IsothermalElectronPressureClosureto takeIons const& ionsis consistent with the summary.169-171: The addition of
NO_DISCARDtoisUsableandisSettableinIsothermalElectronPressureClosureis consistent with the summary.235-235: The summary states that
ions_was changed from a reference to an object, but the code still shows it as a reference. Please verify this discrepancy.236-236: The change of
Te_from a constant reference to a constant inIsothermalElectronPressureClosureis consistent with the summary.237-237: The summary states that
Pe_was changed from a pointer to an object, but the code still shows it as a pointer. Please verify this discrepancy.252-254: The change to the constructor of
ElectronMomentModelto takeIons& ionsis consistent with the summary.304-304: The summary indicates a change in
fluxComput_fromFluxComputertoFluxComputer, which seems to be an error as it suggests no actual change.305-305: The change of
pressureClosure_fromIsothermalElectronPressureClosure<FluxComputer>toIsothermalElectronPressureClosure<Ions>inElectronMomentModelis consistent with the summary.323-324: The addition of copy constructor and deletion of move constructor in
Electronsis consistent with the summary.7-10: The summary indicates significant refactoring related to
GridandFieldtypes, but the provided hunks do not cover all these changes. Please ensure that all related changes are consistent throughout the codebase.src/core/data/field/field.hpp (6)
14-14: The namespace change from
PHAREtoPHARE::coreis correctly applied.25-25: The
usingstatement forSuperis correctly added to aliasNdArrayImpl.31-38: The constructor has been correctly updated to take a
std::arrayfor dimensions.45-46: The
nameandphysicalQuantitymethods have been correctly updated to return references.48-48: The
copyDatamethod should be made private according to the summary, but it is still public in the code.- public: - void copyData(Field const& source) { Super::fill_from(source); } + private: + void copyData(Field const& source) { Super::fill_from(source); }
- 58-68: The new
averagefunction has been correctly added to the code.src/core/data/grid/gridlayout.hpp (7)
325-332: The summary of changes mentions
Field_treplacingNdArrayImpl, but the pull request summary discusses a transition fromFieldtoGridtypes. Please verify the consistency between the summaries and the actual changes in the code.328-332: The change to
physicalStartIndexto accept aField_ttype is consistent with the summary and seems correct.370-374: The change to
physicalEndIndexto accept aField_ttype is consistent with the summary and seems correct.408-412: The change to
ghostStartIndexto accept aField_ttype and use the[[maybe_unused]]attribute for the parameters is consistent with the summary and seems correct.450-453: The change to
ghostEndIndexto accept aField_ttype is consistent with the summary and seems correct.467-473: The change to
fieldNodeCoordinatesto accept aField_ttype is consistent with the summary and seems correct.961-965: The change to
fieldCenteringto accept aField_ttype is consistent with the summary and seems correct.src/core/data/ndarray/ndarray_vector.hpp (6)
197-197: The summary indicates that the 'zero' member function was removed from 'NdArrayVector', but it is actually present in 'NdArrayView'. Please verify if this is an oversight in the summary or the code.
147-150: The use of 'std::decay_t' in the constructor ensures that cv-qualifiers and references are removed from the type, which is a good practice for type compatibility.
184-189: The 'fill_from' function correctly checks for size compatibility before copying data, which ensures data integrity when copying from another 'NdArrayView'.
197-197: The addition of the 'zero' function is a useful feature that allows for resetting the data in the 'NdArrayView' to zero.
191-195: The addition of 'begin' and 'end' functions provides iterator functionality to 'NdArrayView', aligning with standard practices for container-like structures.
200-200: The change of the 'ptr_' member variable type to 'pointer_type' is consistent with the removal of the 'Pointer' template parameter and the refactoring described in the summary.
src/core/data/vecfield/vecfield.hpp (9)
30-30: The summary states that the static constexpr member
dimensionhas been removed, but the hunk shows it being added. Please verify this discrepancy.33-33: The summary indicates that the
field_typealias has been removed from the class definition, but the hunk shows it being defined. Please verify this discrepancy.37-37: The change from a deleted to a default copy constructor is consistent with the summary and is a valid change.
40-40: The change from a deleted to a default move assignment operator is consistent with the summary and is a valid change.
66-71: The modification of the return type of
getFieldNamesAndQuantitiestoresources_propertiesis consistent with the summary and is a valid change.127-130: The use of
autofor the return type of thegetComponentmember function is consistent with the summary and is a valid change.144-147: The use of
autofor the return type of thegetComponentmember function is consistent with the summary and is a valid change.36-36: The deletion of the default constructor is not mentioned in the summary. Please verify if this change is intentional.
39-39: The deletion of the copy assignment operator is not mentioned in the summary. Please verify if this change is intentional.
src/core/models/hybrid_state.hpp (3)
75-83: The addition of
Jto the return tuple ofgetCompileTimeResourcesUserListmay require updates to the code where this function is used. Verify that all usages of this function have been updated to handle the additional tuple element.74-85: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-85]
The summary of the pull request mentions a transition from
FieldtoGridtypes, but this file does not show any changes related to that refactoring. Ensure that the changes in this file are consistent with the overall refactoring effort described in the pull request summary.
- 74-85: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-85]
The summary does not mention the addition of the
VecField Jmember to theHybridStateclass, which is present in the hunk. This should be included in the summary to accurately reflect the changes made to the code.src/core/numerics/ohm/ohm.hpp (3)
54-62: The summary states that the return value of
fnis used to update thelayout_member, but the code shows thatfnis called withstateas an argument, and the result is destructured to obtainlayoutand other variables. The summary should be updated to accurately reflect the code's behavior.65-66: The member variables
eta_andnu_are correctly defined as constants, which is good practice for values that should not change after construction.50-67: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [9-11]
The
Ohmclass correctly inherits fromLayoutHolder<GridLayout>, which is consistent with the design of handling layout information.src/core/utilities/types.hpp (4)
72-76: The addition of
type_liststruct with a nested type aliasTupleis correctly implemented and follows the conventions of the file.330-346: The modifications to the
all,any, andnonefunctions to accept an additional template parameterFnwith a default value ofdecltype(to_bool)are correctly implemented and enhance the flexibility of these functions.351-453: The introduction of the
Applystruct,apply_Nfunctions, and thefor_N_R_modeenum class is correctly implemented and provides a useful mechanism for applying functions to a sequence of integers with different return modes.381-453: The addition of the
for_Nfunctions and the functions for working with named tuples is correctly implemented and follows the conventions of the file.src/diagnostic/detail/types/fluid.hpp (1)
- 191-195: The change to use
field.data()instead of&(*field.begin())is a good practice as it avoids potential dereferencing of an invalid iterator and makes the code more readable. Ensure thatwrite_data_set_flatis designed to handle the case wheredata()returns a pointer to an empty container without dereferencing it.src/phare_core.hpp (6)
38-39: The summary states that
Array_twas replaced withGrid_t, butArray_tis still being used within the definition ofGrid_t. Please verify if this is intentional or if the summary needs to be updated.41-41: The addition of
ArrayView_taligns with the summary and seems correctly defined.42-43: The modifications to
VecField_tandField_tto useGrid_tandArrayView_trespectively are consistent with the summary and appear to be correctly implemented.5-5: The inclusion of
grid.hppis noted and seems appropriate for the changes described in the summary.50-51: The type aliases
ParticleArray_tandParticleSoA_tare present and seem to be correctly defined.53-55: The type aliases
MaxwellianParticleInitializer_t,IonPopulation_t,Ions_t, andElectrons_tare present and seem to be correctly defined.src/simulator/phare_types.hpp (1)
- 18-24: The addition of
Grid_tto thePHARE_Typesstruct aligns with the refactoring described in the pull request summary. Ensure that all relevant parts of the codebase are updated to useGrid_twhere appropriate.tests/amr/data/field/coarsening/test_linear_coarsen.hpp (10)
5-8: The addition of
#include "core/data/grid/grid.hpp"is consistent with the summary and necessary for the changes made in the file.55-60: The replacement of
FieldwithGridin theEMDatastruct is consistent with the summary and reflects the refactoring to use the newGridtype.91-97: The replacement of
FieldwithGridin theFieldCoarsenTestDatastruct is consistent with the summary and reflects the refactoring to use the newGridtype.91-91: The change in the
absErrorconstant value is not mentioned in the summary, but it seems to be a minor change and may not require explicit documentation.91-91: The
absErrorconstant within theFieldCoarsenOperatorTeststruct is not mentioned in the summary, but it seems to be a minor change and may not require explicit documentation.91-91: The changes to the
TYPED_TESTandTYPED_TEST_SUITEmacros are not mentioned in the summary, but they seem to be minor and may not require explicit documentation.91-91: The changes to the
createParamfunction template are not mentioned in the summary, but they seem to be minor and may not require explicit documentation.91-91: The changes to the
loadfunction template are not mentioned in the summary, but they seem to be minor and may not require explicit documentation.91-91: The changes to the
coarsenfunction template are not mentioned in the summary, but they seem to be minor and may not require explicit documentation.91-91: The changes to the
checklambda function within theTYPED_TESTare not mentioned in the summary, but they seem to be minor and may not require explicit documentation.tests/amr/data/field/copy_pack/field_data_test_param.hpp (3)
281-281: The change from
FieldtoGridin the type aliasField1Daligns with the refactoring described in the pull request summary.22-22: The inclusion of
core/data/grid/grid.hppis consistent with the changes described in the pull request summary.15-25: The changes in the includes and type alias are part of a larger refactoring effort and do not introduce unnecessary complexity, aligning with the user's preference for minimal and efficient code.
tests/amr/data/field/geometry/test_field_geometry.cpp (3)
10-15: The addition of the include statement for "core/data/grid/grid.hpp" and the modification of
Field1Dto useGridare consistent with the summary and pull request description. Ensure that the rest of the codebase is updated to reflect these changes whereField1Dis used.35-35: The
Field1Dtype alias has been correctly updated to useGridinstead ofField, aligning with the refactoring goal. Ensure that the usage ofField1Dis consistent with this change throughout the codebase.37-37: The
FieldGeometryParamstruct template parameters appear to be compatible with the newField1Dtype alias. Confirm that theGridLayoutTandFieldImplparameters are used correctly in all instances whereFieldGeometryParamis instantiated.tests/amr/data/field/refine/test_field_refine.cpp (1)
- 78-84: The type alias
FieldThas been correctly updated to useGridinstead ofField. Ensure that all usages ofFieldTthroughout the codebase are updated to work with theGridtype if they were previously expecting aFieldtype.tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (2)
20-23: The change from
FieldtoGridtype is correctly reflected in the type aliasFieldND. This aligns with the summary and the broader refactoring effort described in the pull request.51-53: The change from
FieldtoGridtype is correctly reflected in the type aliasFieldNDwithin theTYPED_TESTbody. This is consistent with the changes in the rest of the file and the broader refactoring effort.tests/amr/data/field/refine/test_field_refinement_on_hierarchy.hpp (2)
9-9: The addition of the include statement for "core/data/grid/grid.hpp" aligns with the refactoring changes described in the pull request summary, ensuring that the necessary
Gridtype definitions are available in this file.6-12: No further action is required for this hunk as the rest of the code remains unchanged and the new include does not seem to introduce any conflicts or issues.
tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (2)
12-18: The import statement has been correctly updated to reflect the transition from
FieldtoGrid.48-54: The type alias
FieldNDhas been correctly updated toGrid<NdArrayVector<dim>, HybridQuantity::Scalar>to align with the refactoring fromFieldtoGrid.tests/amr/data/field/variable/test_field_variable.cpp (2)
8-15: The inclusion of
"core/data/grid/grid.hpp"and the use ofGridin the type aliasFieldVarare consistent with the summary's description of the refactoring fromFieldtoGrid.51-53: The update of the type alias
FieldVarto useGridinstead ofFieldis in line with the refactoring described in the summary.tests/amr/models/test_models.cpp (1)
- 35-49: The summary states that definitions for
VecField1D,GridImplYee1D,ParticleArray1D, andGridYee1Dwere removed, but the hunk shows that these types are still present and have been updated. Please verify the accuracy of the summary.tests/amr/resources_manager/test_resources_manager.cpp (3)
5-11: The summary mentions the inclusion of "core/data/grid/grid.hpp" on line 3, but it is actually included on line 8. Please verify the correct line number for the inclusion.
35-48: The type aliases have been updated as described in the summary, and they appear to be correctly defined.
50-50: The function
createInitDictis well-defined and does not appear in the summary, which is acceptable as it may not be considered a key change.tests/amr/resources_manager/test_resources_manager_basic_hierarchy.hpp (3)
31-42: The constructor correctly initializes member variables using the input file data.
47-79: The
initmethod correctly creates and configures aSAMRAI::hier::Boxobject and uses it to create a new patch level in the hierarchy.31-39: The summary should specify that the
BasicHierarchyclass constructor initializesinputDatabase,dimension,patchHierarchyDatabase,gridGeometry, andhierarchymember variables using data parsed from the input file.tests/amr/tagging/test_tagging.cpp (1)
- 161-165: The change from
Field_ttoGrid_tin the type aliasFieldis consistent with the overall refactoring described in the pull request summary. Ensure that all usages of theFieldalias within theTestTaggerstruct and related test cases are updated to work with the newGrid_ttype.tests/core/data/electrons/test_electrons.cpp (9)
5-8: The addition of
grid.hppand the changes in type definitions align with the refactoring to useGridtypes.130-131: The new type aliases
GridNDandVecFieldNDare correctly defined and used.143-154: The global variables
Jx,Jy,Jz,J,Nibuffer,NiProtons,Fxi,Fyi,Fzi,Vex,Vey,Vez,Vix,Viy,Viz,Pe, andpackare correctly declared with the newGridNDtype.138-198: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [186-213]
The constructor has been modified to initialize the new
GridNDvariables andVecFieldND Jwith appropriate types and values.
227-232: The lambda function used to fill the grid data is consistent with the refactoring and is a generic operation that should work as intended.
250-255: The lambda function for filling the grid data in the 2D case is correctly implemented.
280-285: The lambda function for filling the grid data in the 3D case is correctly implemented.
457-470: The test case
ThatElectronsVelocityEqualIonVelocityMinusJis correctly using theGridYeetype to project the current density onto the velocity grid.593-595: The main function for running all tests is correctly implemented.
tests/core/data/field/test_field.hpp (4)
4-14: The summary indicates changes to the function template
testat line 19, but the hunk provided starts at line 4 and does not include line 19. Please verify the accuracy of the summary or the hunk provided.173-193: The summary indicates changes to the type of
field1at line 31, but the hunk provided does not include line 31. Please verify the accuracy of the summary or the hunk provided.173-193: The summary indicates changes to another
testfunction template at line 47, but the hunk provided does not include line 47. Please verify the accuracy of the summary or the hunk provided.173-193: The pull request summary mentions a significant refactoring from
FieldtoGridtypes, but the hunk still shows the use ofFieldtype in the function signatures. Please verify the accuracy of the pull request summary or the hunk provided.tests/core/data/gridlayout/gridlayout_base_params.hpp (4)
13-13: The summary indicates that
#include "gridlayout_utilities.hpp"was removed, but it is still present in the code.22-22: The summary states that the
using Grid_talias was added at line 10, but it is actually at line 22.32-32: The change of the
fieldtype tostd::shared_ptr<Grid_t>aligns with the refactoring to useGridtypes and is correctly implemented.35-38: The change in the
makeIt_function signature to useGrid_tis consistent with the refactoring efforts and is correctly implemented.tests/core/data/gridlayout/gridlayout_laplacian.hpp (5)
2-8: The update of the include statement from
FieldtoGridis consistent with the summary and the overall refactoring effort.25-30: The change from
FieldtoGridfor theJx,Jy, andJzvariables in the 1D Laplacian class is consistent with the summary and the overall refactoring effort.47-52: The change from
FieldtoGridfor theJx,Jy, andJzvariables in the 2D Laplacian class is consistent with the summary and the overall refactoring effort.69-74: The change from
FieldtoGridfor theJx,Jy, andJzvariables in the 3D Laplacian class is consistent with the summary and the overall refactoring effort.25-33: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [2-77]
The changes in the file are part of a larger refactoring effort and do not introduce unnecessary complexity or inefficiency, aligning with the user's preference for minimal and efficient code.
tests/core/data/ions/test_ions.cpp (2)
9-15: The changes made to the includes and type definitions are consistent with the summary and the broader refactoring effort described in the pull request. No issues found.
34-41: The updates to the type aliases
Grid1DandVecField1Dalign with the refactoring goals outlined in the pull request. The code is clear and maintainable.tests/core/data/vecfield/test_main.cpp (8)
3-6: The summary states that
#include "core/data/grid/grid.hpp"was added at line 3, but it is actually included at line 6 in the hunk.27-44: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [26-30]
The summary states that the type of
vf2was changed at line 26, but the hunk shows the change at line 30.
28-28: The summary mentions a change from
NdArrayVectortoNdArrayViewat line 28, but there is no such change in the provided hunk.30-39: The summary states that a new template alias
VecField_twas added at lines 30-33, but it is actually defined at lines 38-39 in the hunk.92-112: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [88-109]
The summary states that
Fieldwas replaced withGridfor various variables at lines 88-101, but the hunk shows these changes at lines 92-109.
103-109: The summary states that
VecField<NdArrayVector<dim>, HybridQuantity>was replaced withVecField_t<dim>forB1D_,B2D_, andB3D_at lines 103-106, but the hunk shows these changes at lines 107-109.303-312: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [299-324]
The summary states that the type of various variables was changed at lines 299-315, but the hunk shows these changes at lines 306-312 and 318-324.
- 92-112: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-324]
The changes in the hunk are consistent with the larger refactoring effort to use
GridandVecField_ttypes as described in the pull request summary.tests/core/numerics/ampere/test_main.cpp (6)
5-8: The inclusion of "core/data/grid/grid.hpp" aligns with the summary stating that the grid module has been added.
35-39: The refactoring of
physicalStartIndexandphysicalEndIndexmethods inGridLayoutMock1Dto takeFieldMockandDirectionas arguments is consistent with the summary.72-72: The addition of the template alias
Grid_tis consistent with the summary and is a good use of type aliasing for code clarity and maintainability.128-143: The changes in
Ampere1DTestto use theGrid_ttype for fields andVecFieldfor vectors, as well as the addition ofdimandinterp_orderstatic variables, are consistent with the summary and improve type consistency and readability.170-185: The changes in
Ampere2DTestto use theGrid_ttype for fields andVecFieldfor vectors, as well as the addition ofdimandinterp_orderstatic variables, are consistent with the summary and improve type consistency and readability.212-228: The changes in
Ampere3DTestto use theGrid_ttype for fields andVecFieldfor vectors, as well as the addition ofdimandinterp_orderstatic variables, are consistent with the summary and improve type consistency and readability.tests/core/numerics/faraday/test_main.cpp (7)
- 29-55: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [29-68]
The summary states that
[[maybe_unused]]attributes were removed fromphysicalStartIndexandphysicalEndIndexmethods, but the hunks do not show any such attributes being removed. Please verify if the summary is accurate or if the changes were not included in the hunks.
5-11: The change from including
"core/data/field/field.hpp"to"core/data/grid/grid.hpp"aligns with the refactoring towards usingGridtypes.29-55: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [29-68]
The update to use
FieldMock<dimension>in theGridLayoutMockstructs is consistent with the refactoring towards more specific type usage.
136-158: The update to use
Grid_tandVecField<Grid_t, HybridQuantity>in theFaraday1DTestclass is consistent with the refactoring towards usingGridtypes.191-213: The update to use
Grid_tandVecField<Grid_t, HybridQuantity>in theFaraday2DTestclass is consistent with the refactoring towards usingGridtypes.246-269: The update to use
Grid_tandVecField<Grid_t, HybridQuantity>in theFaraday3DTestclass is consistent with the refactoring towards usingGridtypes.246-271: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [136-271]
The test classes
Faraday1DTest,Faraday2DTest, andFaraday3DTestare structured to separate concerns for each dimensionality, which is a good practice for clarity and maintainability in unit testing. This aligns with the user's preference for efficient code, as it makes the tests easier to understand and maintain.
| Grid() = delete; | ||
| Grid(Grid const& source) = delete; | ||
| Grid(Grid&& source) = default; | ||
| Grid& operator=(Grid&& source) = delete; | ||
| Grid& operator=(Grid const& source) = delete; |
There was a problem hiding this comment.
The class Grid has a defaulted move constructor but a deleted move assignment operator. It's generally expected that if a class supports move semantics, it should define both the move constructor and move assignment operator to ensure consistency.
- Grid& operator=(Grid&& source) = delete;
+ Grid& operator=(Grid&& source) = default;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.
| Grid() = delete; | |
| Grid(Grid const& source) = delete; | |
| Grid(Grid&& source) = default; | |
| Grid& operator=(Grid&& source) = delete; | |
| Grid& operator=(Grid const& source) = delete; | |
| Grid() = delete; | |
| Grid(Grid const& source) = delete; | |
| Grid(Grid&& source) = default; | |
| Grid& operator=(Grid&& source) = default; | |
| Grid& operator=(Grid const& source) = delete; |
| std::transform(std::begin(f1), std::end(f1), std::begin(f2), std::begin(avg), | ||
| std::plus<double>()); | ||
|
|
||
| std::transform(std::begin(avg), std::end(avg), std::begin(avg), | ||
| [](double x) { return x * 0.5; }); |
There was a problem hiding this comment.
The average function assumes that the Grid will always be working with double types. This may not be the case, and the function should be templated to work with the value_type of the Grid.
- std::transform(std::begin(f1), std::end(f1), std::begin(f2), std::begin(avg),
- std::plus<double>());
+ std::transform(std::begin(f1), std::end(f1), std::begin(f2), std::begin(avg),
+ std::plus<typename Grid<NdArrayImpl, PhysicalQuantity>::value_type>());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.
| std::transform(std::begin(f1), std::end(f1), std::begin(f2), std::begin(avg), | |
| std::plus<double>()); | |
| std::transform(std::begin(avg), std::end(avg), std::begin(avg), | |
| [](double x) { return x * 0.5; }); | |
| std::transform(std::begin(f1), std::end(f1), std::begin(f2), std::begin(avg), | |
| std::plus<typename Grid<NdArrayImpl, PhysicalQuantity>::value_type>()); | |
| std::transform(std::begin(avg), std::end(avg), std::begin(avg), | |
| [](double x) { return x * 0.5; }); |
The average function does not check if the sizes of the input Grid objects are the same before performing the operation. This could lead to undefined behavior if the Grid objects have different sizes.
| auto operator&() const { return &field_; } | ||
|
|
||
| private: | ||
| std::string name_{"No Name"}; |
There was a problem hiding this comment.
The Grid class initializes the name_ member with a default value of "No Name". It would be better to enforce the name to be provided during construction to avoid objects without meaningful names.
| assert(Super::size() == that.size()); | ||
| std::copy(that.data(), that.data() + Super::size(), Super::data()); |
There was a problem hiding this comment.
The copyData method uses assert to check the size of the Grid objects. This check will not be performed in release builds where asserts may be disabled, potentially leading to undefined behavior.
| auto const& [layout, B, E, Bnew] = fn(state); | ||
| this->layout_ = &layout; | ||
| (*this)(B, E, Bnew, dt); |
There was a problem hiding this comment.
The layout_ member variable is being set inside a loop, which could lead to incorrect behavior if the states collection contains different layouts. Verify that all states are expected to have the same layout or refactor the code to handle different layouts appropriately.
| this->layout_ = &layout; | ||
| (*this)(B, E, Bnew, dt); | ||
| } | ||
| } | ||
|
|
||
| private: | ||
| double dt_; |
There was a problem hiding this comment.
The dt_ member variable is used in the BxEq_, ByEq_, and BzEq_ functions but is not set in the new operator() function. Ensure that dt_ is properly initialized before these functions are called.
tests/core/data/grid/test_grid.cpp
Outdated
| { | ||
| EXPECT_EQ(std::string("test"), this->f.name()); | ||
| } | ||
|
|
||
|
|
||
|
|
||
| TYPED_TEST(GenericField1D, physiscalQuantity) | ||
| TYPED_TEST(GenericGrid1D, physiscalQuantity) |
There was a problem hiding this comment.
There is a typo in the test case name physiscalQuantity. It should be corrected to physicalQuantity for clarity and correctness.
- TYPED_TEST(GenericGrid1D, physiscalQuantity)
+ TYPED_TEST(GenericGrid1D, physicalQuantity)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.
| TYPED_TEST(GenericGrid1D, physiscalQuantity) | |
| TYPED_TEST(GenericGrid1D, physicalQuantity) |
There was a problem hiding this comment.
@PhilipDeegan Thank you! 😊
(\_/)
( •_•)
/>❤️
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for (auto& state : states) | ||
| { | ||
| auto const& [layout, B, E, Bnew] = fn(state); | ||
| this->layout_ = &layout; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory
| /* // how to use | ||
| for_N<2>([](auto ic) { | ||
| constexpr auto i = ic(); | ||
| // ... | ||
| }); | ||
| */ |
Check notice
Code scanning / CodeQL
Commented-out code
| for (auto& state : states) | ||
| { | ||
| auto const& [layout, B, J] = fn(state); | ||
| this->layout_ = &layout; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory
| for (auto& state : states) | ||
| { | ||
| auto const& [layout, n, Ve, Pe, B, J, Enew] = fn(state); | ||
| this->layout_ = &layout; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory
| template<typename... Args> | ||
| auto static _J(Args&... args) | ||
| { | ||
| auto const& [Jx, Jy, Jz] = std::forward_as_tuple(args...); |
Check notice
Code scanning / CodeQL
Unused local variable
| template<typename... Args> | ||
| auto static _ions(Args&... args) | ||
| { | ||
| auto const& [Fxi, Fyi, Fzi, Nibuffer, NiProtons, Vix, Viy, Viz, pack] |
Check notice
Code scanning / CodeQL
Unused local variable
| template<typename... Args> | ||
| auto static _J(Args&... args) | ||
| { | ||
| auto const& [Jx, Jy, Jz] = std::forward_as_tuple(args...); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| template<typename... Args> | ||
| auto static _ions(Args&... args) | ||
| { | ||
| auto const& [Fxi, Fyi, Fzi, Nibuffer, NiProtons, Vix, Viy, Viz, pack] |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| for (auto& state : states) | ||
| { | ||
| auto const& [layout, B, J] = fn(state); | ||
| this->layout_ = &layout; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory Warning
| for (auto& state : states) | ||
| { | ||
| auto const& [layout, B, E, Bnew] = fn(state); | ||
| this->layout_ = &layout; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory Warning
| for (auto& state : states) | ||
| { | ||
| auto const& [layout, n, Ve, Pe, B, J, Enew] = fn(state); | ||
| this->layout_ = &layout; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory Warning
PhilipDeegan
force-pushed
the
views
branch
2 times, most recently
from
72f4841 to
2fd735c
Compare
|
closes #778 |
| if (static_cast<std::size_t>(levelNumber) == model_views_.size()) | ||
| model_views_.push_back(solver.make_view(*level, model)); | ||
| else | ||
| model_views_[levelNumber] = solver.make_view(*level, model); |
There was a problem hiding this comment.
I wasn't super sure of the point of the level descriptor system here so I didn't use it.
That might very well be an issue for future usage with varying solvers between levels, tbc
field -> grid more more more
There was a problem hiding this comment.
Review Status
Actionable comments generated: 22
Configuration used: .coderabbit.yaml
Files selected for processing (57)
- res/cmake/dep.cmake (1 hunks)
- res/cmake/test.cmake (1 hunks)
- src/amr/data/field/field_data.hpp (2 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3 hunks)
- src/amr/multiphysics_integrator.hpp (5 hunks)
- src/amr/physical_models/hybrid_model.hpp (2 hunks)
- src/amr/resources_manager/field_resource.hpp (1 hunks)
- src/amr/resources_manager/resources_manager.hpp (2 hunks)
- src/amr/solvers/solver.hpp (5 hunks)
- src/amr/solvers/solver_mhd.hpp (2 hunks)
- src/amr/solvers/solver_ppc.hpp (6 hunks)
- src/amr/solvers/solver_ppc_model_view.hpp (1 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (1 hunks)
- src/amr/tagging/hybrid_tagger.hpp (1 hunks)
- src/core/data/electrons/electrons.hpp (9 hunks)
- src/core/data/field/field.hpp (1 hunks)
- src/core/data/grid/grid.hpp (1 hunks)
- src/core/data/grid/gridlayout.hpp (6 hunks)
- src/core/data/ions/ion_population/ion_population.hpp (4 hunks)
- src/core/data/ions/ions.hpp (2 hunks)
- src/core/data/ndarray/ndarray_vector.hpp (5 hunks)
- src/core/data/vecfield/vecfield.hpp (4 hunks)
- src/core/models/hybrid_state.hpp (1 hunks)
- src/core/numerics/ampere/ampere.hpp (1 hunks)
- src/core/numerics/faraday/faraday.hpp (1 hunks)
- src/core/numerics/ohm/ohm.hpp (1 hunks)
- src/core/utilities/box/box.hpp (1 hunks)
- src/core/utilities/types.hpp (3 hunks)
- src/diagnostic/detail/types/fluid.hpp (1 hunks)
- src/phare_core.hpp (3 hunks)
- src/simulator/phare_types.hpp (1 hunks)
- tests/amr/data/field/coarsening/test_linear_coarsen.hpp (3 hunks)
- tests/amr/data/field/copy_pack/field_data_test_param.hpp (2 hunks)
- tests/amr/data/field/geometry/test_field_geometry.cpp (2 hunks)
- tests/amr/data/field/refine/test_field_refine.cpp (1 hunks)
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (2 hunks)
- tests/amr/data/field/refine/test_field_refinement_on_hierarchy.hpp (1 hunks)
- tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (2 hunks)
- tests/amr/data/field/variable/test_field_variable.cpp (2 hunks)
- tests/amr/models/test_models.cpp (1 hunks)
- tests/amr/resources_manager/CMakeLists.txt (1 hunks)
- tests/amr/resources_manager/input_config.h (1 hunks)
- tests/amr/resources_manager/test_resources_manager.cpp (2 hunks)
- tests/amr/resources_manager/test_resources_manager_basic_hierarchy.hpp (1 hunks)
- tests/amr/tagging/test_tagging.cpp (1 hunks)
- tests/core/data/electrons/test_electrons.cpp (10 hunks)
- tests/core/data/field/test_field.hpp (2 hunks)
- tests/core/data/grid/CMakeLists.txt (1 hunks)
- tests/core/data/grid/test_grid.cpp (9 hunks)
- tests/core/data/gridlayout/gridlayout_base_params.hpp (1 hunks)
- tests/core/data/gridlayout/gridlayout_deriv.hpp (4 hunks)
- tests/core/data/gridlayout/gridlayout_laplacian.hpp (4 hunks)
- tests/core/data/ion_population/test_ion_population.cpp (1 hunks)
- tests/core/data/ions/test_ions.cpp (2 hunks)
- tests/core/data/vecfield/test_main.cpp (5 hunks)
- tests/core/numerics/ampere/test_main.cpp (6 hunks)
- tests/core/numerics/faraday/test_main.cpp (6 hunks)
Files not processed due to max files limit (4)
- tests/core/numerics/interpolator/test_main.cpp
- tests/core/numerics/ion_updater/test_updater.cpp
- tests/core/numerics/ohm/test_main.cpp
- tests/functional/harris/harris_2d_2.py
Files skipped from review due to trivial changes (4)
- res/cmake/dep.cmake
- src/amr/resources_manager/resources_manager.hpp
- tests/amr/resources_manager/input_config.h
- tests/core/data/grid/CMakeLists.txt
Additional comments: 166
res/cmake/test.cmake (1)
- 10-14: The addition of
tests/core/data/gridand related test directories is consistent with the PR's objective of transitioning to aGrid-based system. Ensure that any obsolete test directories, such astests/core/data/field, are removed if they are no longer relevant to avoid maintaining dead code, in line with the user's preference for minimal and efficient code.src/amr/data/field/field_data.hpp (2)
86-91: The changes in the
getFromRestartmethod to usegetDoubleArrayinstead ofgetVectorand the addition of an assertion to check the vector size are good for ensuring data integrity and avoiding potential issues with empty vectors.314-314: The change in the return type of the
getPointermethod fromFieldImpl*toauto*is consistent with modern C++ practices and should maintain the same functionality while allowing for automatic type deduction.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (3)
49-88: The changes to the
FieldUserstruct, including the use ofGridTinstead ofFieldTand the addition of a nested typefield_type, are consistent with the PR objectives and the generated summaries. ThecopyDatamethod's use of a template parameterThatinstead ofFieldT const&is also in line with the refactoring.95-125: The updates to the
HybridHybridMessengerStrategyclass, including the use ofGridTinstead ofFieldTin type aliases and member types, align with the PR objectives and the generated summaries. The changes to theBaseRefineOp,FieldTimeInterp, andBaseCoarsenOptypes to useGridTare also consistent with the transition to a grid-based system.1006-1010: The
FieldUserinstanceNiOldUser_has been correctly updated to useGridTinstead ofFieldT, and theNiOld_member has been changed fromFieldT*tofield_type*, which is consistent with the refactoring effort described in the PR objectives and the generated summaries.src/amr/multiphysics_integrator.hpp (5)
344-351: The logic for updating or pushing new model views in
initializeLevelDatais correct. However, it's important to ensure that the size ofmodel_views_is always in sync with the number of levels. If the number of levels can change dynamically, additional logic may be needed to handle resizing ofmodel_views_.365-374: The logic for registering levels and pushing back new model views in
resetHierarchyConfigurationis correct. However, it's important to ensure that the size ofmodel_views_is always in sync with the number of levels. If the number of levels can change dynamically, additional logic may be needed to handle resizing ofmodel_views_.495-502: The logic for retrieving the model view in
advanceLevelis correct. However, it's important to ensure that themodel_views_vector has been properly initialized before this function is called. If the vector is not correctly initialized, accessing an element could lead to undefined behavior.575-583: The addition of the
model_views_member is consistent with the changes in the functions that manage model views. Ensure that themodel_views_vector is properly managed throughout the class to prevent issues such as out-of-bounds access or memory leaks.807-807: The addition of the
getModelView_function is consistent with the changes in the functions that manage model views. Ensure that this function is used consistently throughout the class to access model views.src/amr/physical_models/hybrid_model.hpp (3)
7-7: The addition of the include statement for
"core/data/grid/grid.hpp"is consistent with the PR's objective to transition to aGrid-based system.37-38: The introduction of the
grid_typealias within theHybridModelclass is consistent with the PR's objective to refactor the codebase to use a newGridclass template.27-43: The reorganization and addition of type aliases within the
HybridModelclass are consistent with the PR's objective to refactor the codebase to use a newGridclass template.src/amr/solvers/solver.hpp (5)
12-28: The addition of the
PHARE::solvernamespace and theISolverModelViewclass is consistent with the PR summary. However, the class is currently empty, which may be intentional for future extensions.44-49: The addition of new typedefs within the
ISolverclass aligns with the PR summary and is a common practice to simplify complex type names.86-92: The method signature changes for
advanceLevelreflect the transition to using the newhierarchy_tandISolverModelView&types, which is consistent with the PR summary.99-103: The method signature change for
allocateto acceptpatch_t&is consistent with the PR summary and reflects the refactoring towards the new type aliases.108-116: The addition of the
make_viewmethod and the constructor change forISolverare consistent with the PR summary. The constructor now takes a string parameter, which is a change from the previous version.src/amr/solvers/solver_mhd.hpp (2)
17-19: The addition of type aliases
patch_t,level_t, andhierarchy_tis consistent with the PR's objective of refactoring and aligns with the broader transition to a grid-based system. These aliases will help standardize types across the codebase.41-56: The changes in method signatures for
allocateandadvanceLeveland the addition of themake_viewmethod are consistent with the PR's objective. However, themake_viewmethod is not implemented and throws a runtime error. Ensure that this is the intended behavior and that there is a plan to implement this method in the future.src/amr/solvers/solver_ppc.hpp (7)
11-17: The inclusion of
"amr/solvers/solver_ppc_model_view.hpp"and the addition ofFaraday_t faraday_1;suggest that theSolverPPCclass is being extended to use a model view for the Faraday update step, which aligns with the PR title "hierarchy views". This change should be verified across the codebase to ensure consistency.99-101: The addition of the
make_viewfunction is a significant change as it introduces a new way of creating views for the solver. This function should be checked to ensure that it is being used correctly whereverSolverPPCinstances are created and that the dynamic cast is safe and appropriate.205-215: The
restoreState_function now usesstd::moveto restore particle states. This is an efficient way to transfer ownership of the particle data without copying. However, it should be ensured that thetmpDomainandpatchGhostmaps are properly managed after the move operation to avoid accessing moved-from objects.227-250: The
advanceLevelfunction orchestrates the sequence of operations for advancing the solver's state. The changes here seem to reflect the new model view system and should be checked to ensure that the order of operations is correct and that all necessary state is preserved between steps.269-275: The
faraday_1operation withinpredictor1_is a new addition and should be checked for correctness. It's important to ensure that the lambda passed tofaraday_1correctly captures the necessary state and that thenewTimeparameter is used appropriately within the function.401-427: The
_debug_log_move_ionsfunction is a new debug utility. It's important to ensure that this function is only compiled in debug builds to avoid performance overhead in release builds. The use ofPHARE_DEBUG_DOmacro suggests this, but it should be verified.433-463: The
moveIons_function has been updated, and it's crucial to ensure that the sequence of updates and ghost fills is correct and that theUpdaterModeis used appropriately. The changes should be checked against the model's requirements for ion movement.src/amr/solvers/solver_ppc_model_view.hpp (1)
- 35-48: The
AmpereTransformerandOhmTransformerclasses are defined but do not contain any implementation. Please verify if this is intentional or if the implementation is pending.src/amr/tagging/default_hybrid_tagger_strategy.hpp (1)
- 42-46: The changes to the
NdArrayViewinstantiation are consistent with the broader refactoring from aField-based to aGrid-based system and the removal of thePointertemplate parameter as described in the PR summary. This change should be verified across the codebase to ensure that all instances ofNdArrayViewinstantiation have been updated accordingly.src/core/data/electrons/electrons.hpp (7)
10-10: The addition of
#include "core/logger.hpp"is consistent with the changes and may be necessary for logging within the file.139-140: The change from references to objects for
ions_andJ_inStandardHybridElectronFluxComputeralters the ownership and lifecycle management of these members. Ensure that this change is intentional and that all necessary adjustments have been made throughout the codebase to accommodate this new ownership model.157-160: The constructor of
IsothermalElectronPressureClosurenow takes anIonsobject instead of aFluxComputerobject. Verify that all instantiations of this class have been updated to pass the correct type of argument.169-171: The
isUsableandisSettablemethods inIsothermalElectronPressureClosureare now const, which is a good practice as it indicates that these methods do not modify the object state.219-220: The
computePressuremethod inIsothermalElectronPressureClosurenow requires aGridLayoutparameter. Ensure that all calls to this method have been updated to include this new parameter.252-255: The constructor of
ElectronMomentModelnow takes anIonsobject instead of a reference. This change in parameter type could have implications on object management and ownership. Verify that this change is intentional and that all necessary adjustments have been made throughout the codebase.324-324: The move constructor for
Electronsis explicitly deleted. Ensure that this is intentional and that there are no attempts to moveElectronsobjects within the codebase, as this would now result in a compilation error.src/core/data/field/field.hpp (2)
58-68: The summary states that the
averagefunction has been moved outside thecorenamespace, but the hunk shows it is still within thecorenamespace. Please verify this discrepancy.31-38: The new constructor for the
Fieldclass is correctly implemented, initializing the base class and member variables as expected.src/core/data/grid/grid.hpp (8)
- 34-35: The move assignment operator is deleted, which is inconsistent with the default move constructor. Consider providing a default move assignment operator to maintain consistency with move semantics.
- Grid& operator=(Grid&& source) = delete; + Grid& operator=(Grid&& source) = default;
- 86-87: The
averagefunction uses a hard-codeddoubletype, which may not match thevalue_typeof theGrid. Consider using thevalue_typeof theGridin thestd::plusoperation.- std::plus<double>()); + std::plus<typename Grid<NdArrayImpl, PhysicalQuantity>::value_type>());
82-90: The
averagefunction does not check if the sizes of the inputGridobjects are the same before performing the operation. Consider adding a size check to prevent undefined behavior.73-73: The
Gridclass initializes thename_member with a default value of "No Name". Consider enforcing the name to be provided during construction to avoid objects without meaningful names.64-64: The
copyDatamethod usesassertto check the size of theGridobjects, which will not be performed in release builds. Consider using a runtime check that remains in release builds.69-70: The overloaded
operator&returns the address offield_, which could be misleading. Consider providing a more explicit method to get the address offield_.20-75: The
Gridclass does not provide any documentation comments. Adding documentation would improve code readability and maintainability.20-75: Given the significant refactoring from
FieldtoGrid, ensure that all related changes are consistent across the codebase and that dependent modules are updated accordingly.src/core/data/grid/gridlayout.hpp (6)
328-332: The function
physicalStartIndexhas been templated withField_tto generalize its use, which aligns with the refactoring goal of making the code more generic.370-374: The function
physicalEndIndexhas been templated withField_tto generalize its use, which aligns with the refactoring goal of making the code more generic.407-413: The function
ghostStartIndexhas been templated withField_tto generalize its use, which aligns with the refactoring goal of making the code more generic.450-453: The function
ghostEndIndexhas been templated withField_tto generalize its use, which aligns with the refactoring goal of making the code more generic.467-473: The function
fieldNodeCoordinateshas been templated withField_tto generalize its use, which aligns with the refactoring goal of making the code more generic. However, the static assertion ensures that the number of indices provided matches the dimension, which is a good practice for compile-time error checking.961-965: The function
fieldCenteringhas been templated withField_tto generalize its use, which aligns with the refactoring goal of making the code more generic.src/core/data/ions/ion_population/ion_population.hpp (5)
24-30: The addition of
grid_typeandfield_typetype aliases, as well as thedimensionconstant, aligns with the PR's objective to transition to a grid-based system. These changes are consistent with the overall refactoring effort and should be beneficial for standardizing data representation.151-157: The addition of
levelGhostParticlesOldandlevelGhostParticlesNewmember functions is consistent with the PR's objective to enhance theIonPopulationclass's functionality. These methods provide access to different states of ghost particles, which could be important for AMR functionalities.170-176: The
constand non-constoverloads forlevelGhostParticlesNeware correctly implemented, following the established pattern in the class for providing access to particle arrays.192-198: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [180-196]
The changes to the
densitymember function, addingconstqualifiers, are consistent with best practices for const-correctness. This ensures that the function does not modify the object state, which is a good practice for accessor methods.
- 151-161: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [24-197]
The changes in the
IonPopulationclass are consistent with the PR's objective to refactor the codebase to a grid-based system. The new member functions and type aliases support this transition and are likely to improve the codebase's maintainability and performance.src/core/data/ions/ions.hpp (3)
26-31: The introduction of
grid_typeas a type alias is consistent with the PR's objective of transitioning to aGrid-based system. This change should be reflected in all places whereIonPopulation::grid_typeis used within theIonsclass.40-41: The addition of default move and copy constructors is a good practice for classes that manage resources, which can help ensure that instances of
Ionsare copied and moved correctly. This is especially important given the presence of pointers likerho_andmassDensity_within the class.37-44: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [26-44]
The changes to the
Ionsclass are consistent with the PR's objective of transitioning to aGrid-based system and do not appear to introduce any new issues or regressions. The addition of default move and copy constructors is a good practice and likely to improve the maintainability and readability of the code. These changes are likely to have a significant impact on the codebase, requiring updates to dependent modules and potentially affecting the behavior of the software.src/core/data/ndarray/ndarray_vector.hpp (7)
131-150: The changes to the
NdArrayViewclass reflect the removal of thePointertemplate parameter and the introduction ofpointer_type. The constructors have been updated accordingly, and new methods for iterator support and zeroing the array have been added. These changes are consistent with the summary provided.184-189: The
fill_frommethod correctly checks for size compatibility before copying data, which is a good error handling practice.197-197: The
zeromethod is implemented efficiently usingstd::fillto zero out the array.191-195: The
beginandendmethods are correctly provided in both const and non-const versions to support iteration over the array.153-159: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [153-169]
The
operator()overloads for element access are updated to use thevieweralias and are consistent with the removal of thePointertemplate parameter.
200-200: The
ptr_member is safely initialized tonullptr, ensuring a valid state upon default construction.201-201: The
nCells_member is not default-initialized, but this is mitigated by the fact that the constructors ofNdArrayViewalways initialize it.src/core/data/vecfield/vecfield.hpp (4)
23-40: The changes to the
VecFieldclass template, including the renaming of the template parameter fromNdArrayImpltoGrid_t, the addition of new member declarations, and the change from deleted to defaulted copy and move constructors and assignment operators, are consistent with the PR's objective of transitioning to a grid-based system. These changes should be verified across the codebase to ensure that all instances ofVecFieldand related classes are updated accordingly.66-71: The
getFieldNamesAndQuantitiesmethod's implementation is consistent with the PR's objective and the summary. It is now using theVecFieldPropertiesstruct to return the properties of the field components, which aligns with the refactoring towards a more grid-centric approach.127-130: The change in the return type of the
getComponentmethod toauto&is consistent with the PR's objective of transitioning to a grid-based system and the summary. This change simplifies the method signature and is in line with modern C++ practices.144-147: The change in the return type of the
getComponentmethod toauto const&for the const version of the method is consistent with the PR's objective and the summary. This change simplifies the method signature and is in line with modern C++ practices.src/core/models/hybrid_state.hpp (1)
- 75-83: The changes to
getCompileTimeResourcesUserListto include theJvariable in the returned tuple are consistent with the PR's objective of transitioning to a grid-based system. Ensure that all dependent code that uses this function is updated to handle the additional tuple element.src/core/numerics/faraday/faraday.hpp (1)
- 50-60: The new
operator()method does not appear to be related to the PR's objective of transitioning from aField-based representation to aGrid-based system. If this method is part of the refactoring, ensure that it aligns with the new grid-based architecture and that any necessary updates are made to use grid types instead of field types.src/core/utilities/types.hpp (3)
72-76: The addition of the
Tuplealias within thetype_liststruct is a straightforward enhancement that allows for easier use ofstd::tuplewith the types provided totype_list.330-346: The change to make
all,any, andnonefunctionsconstexpris a good improvement, enabling compile-time evaluation where possible.360-462: The introduction of the
Applystruct and related functions for compile-time iteration is a sophisticated addition that can enhance the metaprogramming capabilities of the codebase.src/diagnostic/detail/types/fluid.hpp (1)
- 192-197: The change from
&(*field.begin())tofield.data()in thewriteDSlambda function simplifies the way the address of the data buffer is obtained and is consistent with standard C++ behavior for contiguous storage. This change should be verified to ensure that it does not alter the expected behavior of the program, especially if thefieldobject'sdata()method has been overridden or behaves differently from the standard library containers.src/phare_core.hpp (7)
5-5: The inclusion of "core/data/grid/grid.hpp" is consistent with the PR's objective of refactoring to a grid-based system.
38-46: The introduction of
Grid_tandArrayView_ttypes and their usage inVecField_tandField_tis consistent with the PR's objective of transitioning to a grid-based system.35-46: No code removals or replacements are observed in this hunk, which is consistent with the summary provided.
44-44: The
Electromag_ttype alias has been updated to use the newVecField_twhich now uses theGrid_ttype, reflecting the changes in the underlying data structure for electromagnetic fields.45-46: The
YeeLayout_tandGridLayout_ttype aliases remain unchanged, indicating that the grid layout implementation details are not affected by the transition to the new grid system.50-55: The introduction of
ParticleArray_tandParticleSoA_ttype aliases is consistent with the PR's objective of refactoring to accommodate the new grid structures.55-55: The
IonPopulation_ttype alias has been updated to useVecField_tandGridLayout_t, which have been updated to use the new grid types, reflecting the changes in the underlying data structure for ion populations.src/simulator/phare_types.hpp (1)
- 19-24: The addition of the
Grid_ttype alias aligns with the PR's objective of refactoring the codebase to a grid-based system. Ensure that the introduction of this alias is reflected in all relevant parts of the codebase whereGrid_tshould be used.tests/amr/data/field/coarsening/test_linear_coarsen.hpp (4)
5-11: The inclusion of new headers and the removal of old ones is consistent with the transition from
FieldtoGridas mentioned in the PR objectives and summaries. Ensure that the removed headers are no longer needed and that the new headers are correctly providing the necessary functionality.55-61: The change from
FieldtoGridin theEMDatastruct's type definition is consistent with the PR's objective to transition to a grid-based system. Ensure that all usages ofEMDataand itsField_ttype are updated accordingly.91-97: The change from
FieldtoGridin theFieldCoarsenTestDatastruct's type definition is consistent with the PR's objective. Ensure that all usages ofFieldCoarsenTestDataand itsField_ttype are updated accordingly.55-61: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [55-97]
Given the user's preference for minimal and efficient code, it's important to ensure that the changes made here do not introduce unnecessary complexity or redundancy. The changes appear to be straightforward type updates, but it's worth double-checking that these updates have not led to any redundant code or missed opportunities for further optimization.
tests/amr/data/field/copy_pack/field_data_test_param.hpp (2)
15-25: The changes align with the PR's objective of transitioning to a grid-based system, as indicated by the inclusion of the grid header and the update to the type alias.
281-284: The type aliases are updated to reflect the transition to the grid-based system, which is consistent with the PR's objective and the summary.
tests/amr/data/field/geometry/test_field_geometry.cpp (2)
11-15: The inclusion of "core/data/grid/grid.hpp" and related grid headers is consistent with the PR's objective to refactor the codebase to use a grid-based system.
35-35: The change of the type alias "Field1D" to use the new
Gridclass is in line with the PR's objective of transitioning from aField-based to aGrid-based system.tests/amr/data/field/refine/test_field_refine.cpp (1)
- 78-84: The change from
FieldtoGridin the type aliasFieldTis consistent with the PR's objective of refactoring the codebase to use a grid-based system. Ensure that any test logic that depends on the behavior of the originalFieldtype is updated to work with the newGridtype if there are differences in behavior or interface.tests/amr/data/field/refine/test_field_refinement_on_hierarchy.cpp (2)
22-23: The change from
FieldtoGridin the type aliasFieldNDis consistent with the PR objective and the summary provided, indicating a shift to a grid-based system.53-54: The change from
FieldtoGridin the type aliasFieldNDwithin the test case is consistent with the PR objective and the summary provided, indicating a shift to a grid-based system.tests/amr/data/field/refine/test_field_refinement_on_hierarchy.hpp (2)
6-10: The addition of the include statement for "core/data/grid/grid.hpp" is consistent with the PR's objective of refactoring the codebase to use a
Grid-based system instead of aField-based representation. This change is expected as part of the transition and should be reflected in the logic of the test suite.6-12: Please verify that the rest of the test suite has been updated to reflect the transition from
FieldtoGridand that the newGridclass is being utilized as intended. This may involve checking for updates to test cases, type aliases, and logic within the test suite to ensure compatibility with the new grid-based system.tests/amr/data/field/time_interpolate/test_field_data_time_interpolate.cpp (3)
13-18: The changes in the includes reflect the transition from
FieldtoGridas outlined in the PR objectives and summary. This is consistent with the broader refactoring effort across the codebase.48-54: The update of the type alias
FieldNDto useGridinstead ofFieldis consistent with the PR's objective of refactoring the codebase to use a grid-based system.12-18: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [12-54]
Overall, the changes in the test file are consistent with the PR's objective of transitioning to a grid-based system, and no further issues are observed within the provided context.
tests/amr/data/field/variable/test_field_variable.cpp (3)
8-15: The changes in the includes and the template alias
FieldVarare consistent with the PR's objective of refactoring the codebase to use aGrid-based system instead ofField.51-53: The update of the template alias
FieldVarto useGridinstead ofFieldis a key change that aligns with the PR's objective of refactoring the codebase to use aGrid-based system.50-53: The tests have been updated to use the new
FieldVartemplate alias, which now usesGrid. This ensures that the tests are consistent with the changes made to the codebase.tests/amr/models/test_models.cpp (1)
- 29-49: The changes in the type aliases and class instantiations are consistent with the transition to a
Grid-based system as outlined in the PR summary. The newGrid1D,VecField1D,GridImplYee1D,ParticleArray1D,GridYee1D, and related types are correctly defined and used, which aligns with the objective of refactoring the codebase to use a grid-based representation. This should improve the structure and organization of the code.tests/amr/resources_manager/test_resources_manager.cpp (3)
5-11: The inclusion of "core/data/grid/grid.hpp" is consistent with the PR's objective of refactoring to a Grid-based system.
32-48: The changes in type aliases are consistent with the PR's objective of transitioning to a Grid-based system and do not introduce unnecessary complexity, aligning with the user's preference for minimal and efficient code.
50-51: The updates to
createInitDictusingInitFunctionTare consistent with the PR's objective of refactoring and ensure type safety in the initialization dictionary.tests/amr/resources_manager/test_resources_manager_basic_hierarchy.hpp (3)
31-39: Initialization of member variables in the constructor appears correct and aligns with the PR objectives.
54-55: Hard-coded grid size in
initmethod. Ensure this is intentional and aligns with the test's design.60-64: Usage of a static variable
counterIdto generate unique IDs for patch levels appears to be a deliberate design choice.tests/amr/tagging/test_tagging.cpp (2)
164-164: The change from
FieldtoGrid_taligns with the PR's objective of transitioning from aField-based representation to aGrid-based system.161-167: Given the significant refactoring from
FieldtoGrid_t, it's important to ensure that the associated tests are updated and that new tests are added to cover the new functionality and data structures.tests/core/data/electrons/test_electrons.cpp (7)
5-5: The addition of
#include "core/data/grid/grid.hpp"aligns with the PR's objective of transitioning to aGrid-based system.20-20: The addition of
#include <string_view>is likely related to the use ofstd::string_viewin the code, which is a more efficient way to handle strings when copies are not needed.127-136: The changes in the
ElectronsTeststruct, including the addition ofdensityNameand updates to type definitions forGridNDandVecFieldND, are consistent with the PR's objective of refactoring to a grid-based system.146-160: The addition of
GridNDvariables and template member functions_ionsand_Jin theElectronsTeststruct is in line with the PR's objective of enhancing the grid-based functionalities.142-204: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [192-219]
The constructor of
ElectronsTesthas been updated to initialize the newGridNDandVecFieldNDvariables, which is consistent with the PR's objective of refactoring to a grid-based system.
235-238: The
filllambda functions have been updated to initialize the fields for different dimensions, which is necessary due to the changes in the data structures and types.599-601: The
mainfunction is standard for a test suite and has no issues.tests/core/data/field/test_field.hpp (2)
4-14: The addition of
#include "core/data/ndarray/ndarray_vector.hpp"and the changes to the template parameters are consistent with the overall refactoring effort described in the PR summary. Ensure that the new dependency does not introduce any circular dependencies and that all affected code has been updated to reflect the new template parameters.173-193: The addition of new template functions for testing different field types is consistent with the refactoring effort and the transition from
FieldtoGrid. Ensure that these new functions are covered by unit tests to verify their correctness and that they do not introduce any unintended side effects.tests/core/data/grid/test_grid.cpp (6)
5-8: The inclusion of "core/data/grid/grid.hpp" aligns with the PR's objective of transitioning to a Grid-based system.
13-53: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [15-59]
The renaming of test classes and the use of
Gridclass in place ofFieldis consistent with the PR's objective and the summary provided.
135-168: The tests for the
Gridclass are consistent with the changes made in the previous hunks.192-204: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [173-201]
The tests for the
Gridclass are consistent with the changes made in the previous hunks.
242-251: The test for the
averagefunction is consistent with the PR's objective of adding new functionalities to theGridclass.272-281: The tests for the
averagefunction for 2D and 3D grids are consistent with the PR's objective of adding new functionalities to theGridclass.tests/core/data/gridlayout/gridlayout_base_params.hpp (5)
24-24: The summary indicates that
std::shared_ptr<GridLayout<GridLayoutImpl>> layout;should have been replaced withstd::shared_ptr<Grid_t> layout;, but the code still shows the originallayoutdeclaration. Please verify if this is an oversight in the summary or if the code change was missed.32-32: The change from
FieldtoGridfor thefieldmember is consistent with the PR objectives and the summary.35-39: The update to the
makeIt_function to useGrid_tis consistent with the PR objectives and the summary.41-41: The
makeMyField_function indirectly reflects the transition toGridby utilizing the updatedmakeIt_function.17-41: The refactoring of the
GridLayoutTestParamstruct to use theGridclass template aligns with the user's preference for minimal and efficient code.tests/core/data/gridlayout/gridlayout_deriv.hpp (4)
5-8: The changes in the header files are consistent with the PR objectives and the summary provided, indicating a shift from
FieldtoGridin the codebase.27-31: The replacement of
FieldwithGridfor theByandEzvariables in thea1DDerivativeclass is consistent with the PR objectives and the summary provided.47-51: The replacement of
FieldwithGridfor theByandEzvariables in thea2DDerivativeclass is consistent with the PR objectives and the summary provided.67-71: The replacement of
FieldwithGridfor theByandEzvariables in thea3DDerivativeclass is consistent with the PR objectives and the summary provided.tests/core/data/gridlayout/gridlayout_laplacian.hpp (4)
2-8: The changes in the import statements align with the PR objectives and the generated summaries, indicating a shift from
FieldtoGridbased system.25-30: The variable type changes from
FieldtoGridforJx,Jy, andJzin the 1D case are consistent with the PR objectives and the generated summaries.47-52: The variable type changes from
FieldtoGridforJx,Jy, andJzin the 2D case are consistent with the PR objectives and the generated summaries.69-74: The variable type changes from
FieldtoGridforJx,Jy, andJzin the 3D case are consistent with the PR objectives and the generated summaries.tests/core/data/ion_population/test_ion_population.cpp (1)
- 24-30: The addition of
grid_typewithin theDummyVecFieldstruct aligns with the broader refactoring effort fromFieldtoGridand is consistent with the PR objectives. The change is localized to this test file and does not appear to affect external modules.tests/core/data/ions/test_ions.cpp (5)
9-15: The addition of the include statement for "core/data/grid/grid.hpp" aligns with the PR's objective of transitioning to a Grid-based system.
37-38: The modification of the type alias
VecField1Dto useGrid1Dis consistent with the PR's objective of refactoring to a Grid-based system.37-37: The introduction of the type alias
Grid1Dis consistent with the PR's objective of refactoring to a Grid-based system.38-38: The update of the type
VecField1Dto useGrid1Dis consistent with the PR's objective of refactoring to a Grid-based system.41-41: The type
IonPopulation1Dcorrectly usesVecField1Das indicated in the PR summary. The summary contains an error in stating thatIonPopulation1Dwas changed to useGridYee1Dinstead ofVecField1D.tests/core/data/vecfield/test_main.cpp (6)
6-9: The inclusion of both
grid.hppandfield.hppheaders suggests thatFieldrelated code might still be present in the file or the codebase. Since the PR objective is to transition fromFieldtoGrid, verify that the inclusion offield.hppis still necessary.27-30: The
VecFieldGenericclass correctly uses theGridclass template, aligning with the PR objective to transition fromFieldtoGrid.38-39: The introduction of the
VecField_ttemplate alias is consistent with the PR objective and the summary provided. It replaces theVecFieldinstances with aGrid-based type.92-109: The
VecFieldTestclass correctly uses theVecField_ttemplate alias andGridwithNdArrayVector, which is consistent with the PR objective to transition fromFieldtoGrid.303-312: The test case
TEST(aVecField, dataCanBeCopiedIntoAnother)correctly usesVecField_tandGridwithNdArrayVector, which is consistent with the PR objective to transition fromFieldtoGrid.318-324: The test case
TEST(aVecField, dataCanBeCopiedIntoAnother)also correctly usesVecField_tandGridwithNdArrayVectorfor the second set of grids, aligning with the PR objective to transition fromFieldtoGrid.tests/core/numerics/ampere/test_main.cpp (7)
5-11: The inclusion of both
grid.hppandfield.hppsuggests thatFieldtypes may still be in use, which could be inconsistent with the PR's objective to transition fully toGrid. Verify ifFieldtypes are still needed or if their inclusion is a leftover from before the refactoring.35-40: The refactoring of
GridLayoutMockstructs is not mentioned in the summary. If these changes are significant, the summary should be updated to reflect them.71-72: The introduction of the
Grid_talias and its usage in the test classes is consistent with the PR's objective to transition to aGrid-based system.125-143: The update of the
Amperetest classes to useGrid_tandVecField<Grid_t>is consistent with the PR's objective to transition to aGrid-based system.167-185: The update of the
Amperetests for 2D cases to reflect the newGridsystem is consistent with the PR's objective to transition to aGrid-based system.209-228: The update of the
Amperetests for 3D cases to reflect the newGridsystem is consistent with the PR's objective to transition to aGrid-based system.5-11: The summary of generated summaries mentions the removal of the pre-increment operator method from the
box_iteratorclass in theBoxclass. Verify this change in the relevant files.tests/core/numerics/faraday/test_main.cpp (5)
5-11: The inclusion of grid-related headers aligns with the PR objective of transitioning to a
Grid-based system.29-55: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [29-68]
The
GridLayoutMockstructs still useFieldMockinstead of the newGrid-based system. Verify ifFieldMockneeds to be updated to align with the newGridsystem.
- 136-160: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [136-270]
The test classes have been updated to use
Grid_t, which is consistent with the PR objective of transitioning to aGrid-based system.
- 246-271: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [70-322]
The test cases appear to be correctly testing the
Faradayfunctionality with the newGridsystem.
- 5-11: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [322-328]
The main function for running the tests is correctly set up.
| using patch_data_type = FieldData<GridLayoutT, typename ResourcesUser::field_type>; | ||
| using variable_type = FieldVariable<GridLayoutT, typename ResourcesUser::field_type>; | ||
| using patch_data_type = FieldData<GridLayoutT, typename ResourcesUser::grid_type>; | ||
| using variable_type = FieldVariable<GridLayoutT, typename ResourcesUser::grid_type>; | ||
| using internal_type_ptr = typename ResourcesUser::field_type*; |
There was a problem hiding this comment.
The internal_type_ptr type alias still references typename ResourcesUser::field_type. If the transition from Field to Grid is complete, this might be an oversight and should be updated to use grid_type instead.
- using internal_type_ptr = typename ResourcesUser::field_type*;
+ using internal_type_ptr = typename ResourcesUser::grid_type*;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.
| using internal_type_ptr = typename ResourcesUser::field_type*; | |
| using internal_type_ptr = typename ResourcesUser::grid_type*; |
| std::transform(std::begin(f1), std::end(f1), std::begin(f2), std::begin(avg), | ||
| std::plus<double>()); |
There was a problem hiding this comment.
The average function uses std::plus<double>(), which assumes the Field's value type is double. If Field can have other value types, this could lead to incorrect behavior. Consider using the value_type of Field instead.
| auto dim | ||
| = static_cast<unsigned short int>(inputDatabase->getDatabase("Main")->getInteger("dim")); |
There was a problem hiding this comment.
Redundant retrieval of dimension from inputDatabase in the init method, as it is already stored in the dimension member variable.
- auto dim = static_cast<unsigned short int>(inputDatabase->getDatabase("Main")->getInteger("dim"));
+ auto dim = dimension;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.
| auto dim | |
| = static_cast<unsigned short int>(inputDatabase->getDatabase("Main")->getInteger("dim")); | |
| auto dim = dimension; |
| increment(dim - 1); | ||
| return *this; | ||
| } | ||
| box_iterator& operator++() const | ||
| { | ||
| increment(dim - 1); | ||
| return *this; | ||
| } | ||
|
|
||
|
|
||
| bool operator!=(box_iterator const& other) const | ||
| { |
There was a problem hiding this comment.
The pre-increment operator method box_iterator& operator++() is no longer marked as const. This change may affect how the iterator is used with const instances of Box. Please verify that this change is intentional and does not negatively impact the usage of box_iterator in contexts where const correctness is expected.
| NO_DISCARD ParticleArray const& levelGhostParticlesOld() const | ||
| { | ||
| return const_cast<IonPopulation*>(this)->levelGhostParticlesOld(); |
There was a problem hiding this comment.
The use of const_cast in the const overloads of levelGhostParticlesOld, levelGhostParticlesNew, and density is a common pattern to avoid code duplication. However, it's important to ensure that the non-const versions of these functions do not perform any non-const operations on the object to maintain const-correctness.
Also applies to: 173-175, 193-195
| auto gsi_X = this->layout.ghostStartIndex(field, Direction::X); | ||
| auto gei_X = this->layout.ghostEndIndex(field, Direction::X); |
There was a problem hiding this comment.
The same copy-paste error is present in the 3D fill lambda function where Viy and Jy are filled twice, and Viz and Jz are not filled at all. This should be corrected to ensure that all components are initialized properly.
- fill(Viy, [](double x, double y, double z) {
- return std::cosh(0.4 * x) * std::cosh(0.4 * y) * std::cosh(0.4 * z);
- });
- fill(Jy, [](double x, double y, double z) {
- return std::sinh(0.4 * x) * std::sinh(0.4 * y) * std::sinh(0.4 * z);
- });
+ fill(Viz, [](double x, double y, double z) {
+ return std::cosh(0.4 * x) * std::cosh(0.4 * y) * std::cosh(0.4 * z);
+ });
+ fill(Jz, [](double x, double y, double z) {
+ return std::sinh(0.4 * x) * std::sinh(0.4 * y) * std::sinh(0.4 * z);
+ });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.
| auto gsi_X = this->layout.ghostStartIndex(field, Direction::X); | |
| auto gei_X = this->layout.ghostEndIndex(field, Direction::X); | |
| auto gsi_X = this->layout.ghostStartIndex(field, Direction::X); | |
| auto gei_X = this->layout.ghostEndIndex(field, Direction::X); |
| @@ -42,7 +42,7 @@ void DefaultHybridTaggerStrategy<HybridModel>::tag(HybridModel& model, | |||
|
|
|||
| // SAMRAI tags int* buffer is FORTRAN ordering so we set false to the view | |||
| bool constexpr c_ordering = false; | |||
| auto tagsv = core::NdArrayView<dimension, int, int*, c_ordering>(tags, layout.nbrCells()); | |||
| auto tagsv = core::NdArrayView<dimension, int, c_ordering>(tags, layout.nbrCells()); | |||
|
|
|||
| if constexpr (dimension == 1 and false) | |||
There was a problem hiding this comment.
The if constexpr block with the condition dimension == 1 and false will never execute and appears to be dead code. Consider removing it to clean up the codebase, especially given the preference for minimal and efficient code.
- if constexpr (dimension == 1 and false)
- {
- }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.
| if constexpr (dimension == 1 and false) |
| operator bool() const | ||
| { | ||
| using Tuple = decltype((*this)()); | ||
| auto constexpr tuple_size = std::tuple_size_v<Tuple>; | ||
| Tuple tup = (*this)(); | ||
|
|
||
| return core::for_N_all<tuple_size>([&](auto i) { | ||
| auto const& [k, v] = std::get<i>(tup); | ||
| auto isUsable = v->isUsable(); | ||
| if (!isUsable) | ||
| { | ||
| PHARE_LOG_LINE_STR("NOT USABLE: " << k); | ||
| } | ||
| return isUsable; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The conversion operator to bool in PatchState_t performs a complex check which could be a performance concern if used frequently. Consider optimizing this if it's a common operation.
| auto& operator*() | ||
| { | ||
| assert(view.states[idx] == true); | ||
| return view.states[idx]; | ||
| } | ||
| auto& operator*() const | ||
| { | ||
| assert(view.states[idx] == true); | ||
| return view.states[idx]; |
There was a problem hiding this comment.
The assert statement in the iterator's dereference operator is good for debug builds but might be unnecessary overhead in release builds. Consider using a debug-only assert or providing a way to disable it in release builds.
| return *this; | ||
| } | ||
|
|
||
| bool operator!=(iterator const& that) const { return &view != &that.view or idx != that.idx; } |
There was a problem hiding this comment.
The not-equal operator in the iterator class template should only compare the index if the views are the same, not the pointers to the views.
- bool operator!=(iterator const& that) const { return &view != &that.view or idx != that.idx; }
+ bool operator!=(iterator const& that) const { return &view == &that.view && idx != that.idx; }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.
| bool operator!=(iterator const& that) const { return &view != &that.view or idx != that.idx; } | |
| bool operator!=(iterator const& that) const { return &view == &that.view && idx != that.idx; } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- src/amr/solvers/solver_ppc.hpp (6 hunks)
Additional comments: 8
src/amr/solvers/solver_ppc.hpp (8)
11-17: The addition of
solver_ppc_model_view.hppaligns with the summary's mention of new include directives and the introduction of model views.56-56: Verify the usage of the new member variable
faraday_1within the class to ensure it's correctly integrated and utilized.214-215: Ensure that the keys used in
restoreState_always exist intmpDomainandpatchGhostto prevent potential exceptions when keys are not found.227-251: The changes in
advanceLevelseem to follow a logical sequence of operations for advancing the simulation level. Ensure that the order of operations is correct and aligns with the intended simulation behavior.257-297: The changes in
predictor1_,predictor2_, andcorrector_functions should be verified for correctness, ensuring they align with the intended simulation behavior and the newGridclass integration.401-429: Verify the usage and invocation of the
_debug_log_move_ionsdebug function to ensure it's correctly integrated and can be controlled or disabled as needed.433-463: The changes in
moveIons_should be verified for correctness, ensuring that ion updates and ghost particle management align with the intended simulation behavior and the newGridclass integration.219-472: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-472]
The provided hunks do not show the shift from
FieldtoGridas mentioned in the summary. Ensure that the changes in theSolverPPCclass are consistent with the broader refactor across the codebase.
| { | ||
| for (auto& patch : level) | ||
| for (auto& state : view) | ||
| { | ||
| std::stringstream ss; | ||
| ss << patch->getGlobalId(); | ||
|
|
||
| auto _ = rm.setOnPatch(*patch, ions); | ||
| for (auto& pop : ions) | ||
| ss << state.patch->getGlobalId(); | ||
| for (auto& pop : state.ions) | ||
| { | ||
| auto key = ss.str() + "_" + pop.name(); | ||
| if (!tmpDomain.count(key)) |
There was a problem hiding this comment.
The use of stringstream for string concatenation in saveState_ could be inefficient, especially in a multi-threaded context. Consider using a more efficient string concatenation method if performance is a concern.
|
closing this while I finish it just to not trigger any builds on commit |
for #778
Summary by CodeRabbit
Refactor
FieldtoGridacross various components, enhancing the data structure and processing efficiency.Bug Fixes
New Features
Gridclass with advanced capabilities and integrated it into the system.Documentation
FieldtoGrid.Tests
Gridfunctionality and ensure compatibility.