add Pressure diag #772

nicolasaunai · 2023-11-07T10:31:22Z

Computing the pressure is like using the ParticleToMesh except we need to deposit neither 1 nor the velocity, but the velocity squared. The idea thus is to generalize the ParticleToMesh operator so, per particle, to deposit only 1 field at a time, given the field, the particle and a functor that, given the particle would:

for the density, return 1
for the flux, return vx, or vy or vz
for the pressure, return vxvx, vxvy etc.
This is done so that one day if needed to compute something else, like the heat flux (v^3) we can simply add more functors.

Summary by CodeRabbit

New Features
- Introduced a new TensorField class template to represent and manipulate tensor fields.
- Added search functionality with a new search bar component for improved user experience.
- Expanded the set of predefined constants for scalar quantities to enhance data representation.
Enhancements
- Upgraded the VecField class to utilize TensorField, streamlining the codebase.
- Enhanced the IonPopulation and Ions classes with momentum tensor capabilities.
- Improved the Interpolator and ParticleToMesh classes for more generic and versatile operations.
Bug Fixes
- Fixed issues related to momentum tensor calculations in various diagnostic classes.
- Addressed inconsistencies in diagnostic data collection and representation.
Documentation
- Updated documentation to reflect new features and changes in diagnostic data handling.
Refactor
- Refactored the ElectromagDiagnosticWriter class for more efficient tensor field data handling.
- Codebase cleanup by removing redundant includes and updating dependencies.
Tests
- Added and updated tests to cover new tensor field functionalities and momentum tensor calculations.
Chores
- Modified build configurations to enable development mode for improved developer experience.
Style
- Updated code styling for consistency across the project.
Revert
- No reverts in this release.

Note: The above release notes have been crafted to focus on end-user impact and do not reveal internal code structures or file names.

coderabbitai · 2023-11-07T10:31:56Z

Walkthrough

The changes involve refactoring and extending the functionality of tensor fields in a C++ codebase, likely for scientific computation or simulation purposes. The TensorField and VecField classes have been updated, with VecField becoming an alias for TensorField. New tensor-related functionalities are integrated, impacting various classes and tests. The codebase also adapts to handle momentum tensors, with updates to diagnostics and interpolators. Build configurations and Python bindings are modified to align with these changes.

Changes

File Path	Summary of Changes
`src/core/data/tensorfield/tensorfield.hpp` `src/core/data/vecfield/vecfield.hpp` `src/core/data/ions/ion_population/ion_population.hpp` `src/core/data/grid/...` `src/hybrid/hybrid_quantities.hpp` `src/numerics/interpolator/interpolator.hpp` `src/diagnostic/detail/...`	Refactoring and extension of tensor field functionalities, introduction of momentum tensor handling in various classes, and updates to interpolators and diagnostics.
`src/core/CMakeLists.txt`	Addition of `tensorfield.hpp` to the project build configuration.
`.github/workflows/cmake_ubuntu.yml`	Addition of a development mode flag `-DdevMode=ON` to the cmake command in the build job.
`tests/...`	Updates to test files reflecting the new tensor field logic and momentum tensor handling.
`src/phare_core.hpp` `tests/amr/models/test_models.cpp` `tests/amr/resources_manager/...` `tests/core/data/electrons/...` `tests/core/data/ion_population/...` `tests/core/data/ions/...` `tests/core/numerics/ion_updater/...`	Changes to type declarations and template instantiations to accommodate the new tensor field types.
`pyphare/...`	Python bindings and scripts updated to reflect changes in tensor fields and diagnostics.
`src/restarts/detail/h5writer.hpp` `src/diagnostic/diagnostics.hpp`	Changes to dependencies and writer types in diagnostics-related files.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ?

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

You can reply to a review comment made by CodeRabbit.
You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

coderabbitai

Review Status

Actionable comments generated: 11

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 32ae294 and eaf388d.

Files selected for processing (27)

src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 hunks)
src/amr/physical_models/hybrid_model.hpp (1 hunks)
src/core/CMakeLists.txt (1 hunks)
src/core/data/grid/gridlayoutdefs.hpp (1 hunks)
src/core/data/grid/gridlayoutimplyee.hpp (4 hunks)
src/core/data/ions/ion_population/ion_population.hpp (8 hunks)
src/core/data/ions/ions.hpp (9 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield.hpp (2 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/core/hybrid/hybrid_quantities.hpp (2 hunks)
src/core/numerics/interpolator/interpolator.hpp (10 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (2 hunks)
src/diagnostic/detail/types/fluid.hpp (11 hunks)
src/diagnostic/detail/types/particle.hpp (1 hunks)
src/diagnostic/diagnostics.hpp (1 hunks)
src/phare_core.hpp (3 hunks)
src/python3/patch_level.hpp (6 hunks)
src/restarts/detail/h5writer.hpp (1 hunks)
tests/amr/models/test_models.cpp (3 hunks)
tests/amr/resources_manager/test_resources_manager.cpp (3 hunks)
tests/core/data/electrons/test_electrons.cpp (6 hunks)
tests/core/data/ion_population/test_ion_population.cpp (3 hunks)
tests/core/data/ions/test_ions.cpp (2 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
tests/diagnostic/test_diagnostics.hpp (2 hunks)

Files skipped from review due to trivial changes (5)

src/amr/physical_models/hybrid_model.hpp
src/core/CMakeLists.txt
src/core/data/grid/gridlayoutdefs.hpp
src/python3/patch_level.hpp
tests/amr/resources_manager/test_resources_manager.cpp

Additional comments: 85

src/diagnostic/diagnostics.hpp (1)

55-61: Ensure that the change from h5::Writer<ModelView_t> to h5::H5Writer<ModelView_t> does not break any existing functionality. Verify that the new h5::H5Writer<ModelView_t> class has the same interface or that all uses of Writer_t have been updated to match the new interface.

tests/diagnostic/test_diagnostics.hpp (2)

36-40: The change from core::Components::componentMap to core::Components::componentMap() is correct if componentMap is a function and not a data member. Ensure that this change is consistent with the definition in the core::Components namespace.

45-48: The change from Writer<ModelView_t> to H5Writer<ModelView_t> is correct if H5Writer is a valid type and is intended to be used here. Ensure that this change is consistent with the definition in the diagnostic::h5 namespace.

tests/amr/models/test_models.cpp (1)

33-47: The addition of SymTensorField1D and its inclusion in the template parameters of IonsPop1D, HybridModelT, and MHDModelT is consistent with the pull request summary. Ensure that these changes are propagated throughout the codebase where these types are used.

src/phare_core.hpp (2)

33-45: The addition of SymTensorField_t type alias is consistent with the pull request summary. Ensure that this new type is used correctly throughout the codebase.

49-58: The modification of IonPopulation_t to include SymTensorField_t as a template parameter is consistent with the pull request summary. Ensure that all instances of IonPopulation_t in the codebase have been updated to match the new template signature.

tests/core/data/ion_population/test_ion_population.cpp (2)

29-38: The DummyTensorField struct is a good addition for testing purposes. It's important to ensure that the isUsable and isSettable methods return the correct values for the test cases.

60-61: The AnIonPopulation struct has been updated to use DummyTensorField. Ensure that this change doesn't affect other tests that rely on this struct.

src/core/data/vecfield/vecfield.hpp (2)

6-21: The VecField class has been replaced with an alias for TensorField. This change simplifies the code and reduces redundancy. However, ensure that all instances of VecField in the codebase are compatible with the TensorField interface.

29-34: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [20-31]

The average function has been removed. Make sure that this function is not used elsewhere in the codebase, or that an equivalent function exists in the TensorField class.

src/diagnostic/detail/types/particle.hpp (1)

4-11: The addition of core/data/grid/gridlayout.hpp and the removal of amr/data/particles/particles_data.hpp are not reflected in the provided hunk. Please verify if the changes are correctly reflected in the code.

src/restarts/detail/h5writer.hpp (8)

1-2: Ensure that the header guard is unique across the codebase.

5-8: Ensure that the new includes are necessary and that the removed includes are not required elsewhere in this file.

10-10: Ensure that the namespace is correctly defined and does not conflict with other namespaces in the codebase.

1-10: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [14-20]

Ensure that the constructor is being called with the correct arguments throughout the codebase.

1-10: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [22-30]

Ensure that the make_unique function is being called with the correct arguments throughout the codebase.

1-10: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [32-49]

Ensure that the dump function is being called with the correct arguments throughout the codebase.

1-10: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [51-53]

Ensure that the modelView function is being called correctly throughout the codebase.

1-10: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [55-57]

Ensure that the private members are being used correctly within the class.

src/amr/data/field/coarsening/field_coarsen_operator.hpp (1)

68-74: The return type of getStencilWidth has been changed from SAMRAI::hier::IntVector to SAMRAI::hier::IntVector, which is correct. However, the comment above the function indicates that a ghost width of 5 is sufficient, but the function returns a vector with all components set to 2. Please verify this discrepancy.

src/core/data/grid/gridlayoutimplyee.hpp (5)

94-105: The new arrays for Mxx, Mxy, Mxz, Myy, Myz, and Mzz are initialized correctly. Ensure that the data.primal values are correctly set before this point.

109-112: The hybridQtyCentering array is updated to include the new quantities. Ensure that all uses of this array throughout the codebase have been updated to handle the new quantities.

167-178: The return values of the function have been updated to include the new quantities. Ensure that all calls to this function throughout the codebase have been updated to handle the new return values.

229-246: The return values of the function have been updated to include the new quantities. Ensure that all calls to this function throughout the codebase have been updated to handle the new return values.

311-334: The return values of the function have been updated to include the new quantities. Ensure that all calls to this function throughout the codebase have been updated to handle the new return values.

src/core/data/vecfield/vecfield_component.hpp (3)

12-12: The new enum values XX, XY, XZ, YY, YZ, and ZZ have been added to the Component enum class. Ensure that these new values are handled correctly throughout the codebase.

18-24: The componentMap function now throws a std::runtime_error if the rank is not 1 or 2. This is a good practice as it prevents misuse of the function.

35-41: The check function has been updated to include checks for the new enum values. This is a good practice as it ensures that only valid enum values are used.

src/core/hybrid/hybrid_quantities.hpp (3)

13-38: The addition of new enum values for momentum tensor components and pressure is a good update. Ensure that these new enum values are used correctly throughout the codebase.

42-43: The TensorType template alias is a good addition. It provides a way to handle both vector and tensor quantities in a unified manner.

45-46: The componentsQuantities function is used correctly here to return the components of the vector quantities B and E.
src/core/data/ions/ion_population/ion_population.hpp (9)
6-9: The inclusion of <array> is new and is used for the change from std::vector to std::array in MomentProperties and ParticleProperties.

27-30: The TensorField type is introduced and VecField is replaced with TensorField in the IonPopulation class template.

33-40: The constructor now initializes a new momentumTensor_ member.
+ , momentumTensor_{name_ + "_momentumTensor", HybridQuantity::Tensor::M}
51-63: The isUsable and isSettable functions now include checks for momentumTensor_.
+ && momentumTensor_.isUsable()
+ && momentumTensor_.isSettable()
191-195: New getter and setter methods for momentumTensor_.
+ TensorField const& momentumTensor() const { return momentumTensor_; }
+ TensorField& momentumTensor() { return momentumTensor_; }
208-214: MomentProperties is now a std::array instead of a std::vector.
- using MomentProperties = std::vector<MomentsProperty>;
+ using MomentProperties = std::array<MomentsProperty, 1>;
226-231: ParticleProperties is now a std::array instead of a std::vector.
- using ParticleProperties = std::vector<ParticleProperty>;
+ using ParticleProperties = std::array<ParticleProperty, 1>;
260-264: The getCompileTimeResourcesUserList function now includes momentumTensor_.
+ return std::forward_as_tuple(flux_, momentumTensor_);
283-289: The momentumTensor_ member is added to the IonPopulation class.
+ TensorField momentumTensor_;
tests/core/data/electrons/test_electrons.cpp (6)

1-10: The new include statement for "core/data/tensorfield/tensorfield.hpp" is added. Ensure that the path is correct and the file exists.

126-139: The new type SymTensorFieldND is introduced and the IonPopulationND type is modified to include SymTensorFieldND. Ensure that these changes are reflected in all parts of the code where these types are used.

149-166: Several new fields related to momentum tensors are added. Make sure that these fields are initialized properly and used correctly throughout the code.

188-217: The buffer assignments for momentum tensors are updated. Ensure that the sizes allocated for these buffers are correct and that they are properly deallocated when no longer needed.

232-241: The momentum tensor buffers are set for the ions object. Ensure that the ions object is properly initialized before these operations.

247-255: The momentum tensor buffers are set for the pops[0] object. Ensure that the pops[0] object is properly initialized before these operations.

src/core/data/ions/ions.hpp (7)

7-11: The new includes for <vector> and <array> are appropriate for the changes made in this file.

28-34: The addition of tensorfield_type as a member type in the Ions class is consistent with the changes made in this file.

41-45: The addition of momentumTensor_ as a member variable in the Ions class is consistent with the changes made in this file.

94-96: The addition of momentumTensor() member functions to get and set the momentum tensor is consistent with the changes made in this file.

204-210: The change of MomentProperties from std::vector to std::array is a good optimization if the size of MomentProperties is known at compile time and does not change.

234-238: The addition of getCompileTimeResourcesUserList() function is consistent with the changes made in this file.

259-264: The addition of momentumTensor_ as a private member variable in the Ions class is consistent with the changes made in this file.

src/core/numerics/interpolator/interpolator.hpp (5)

277-296: The ParticleToMesh class has been updated to include a new template parameter Func for the function to calculate the deposit. This change allows for more flexibility in the calculations performed by the class. Ensure that all calls to this class throughout the codebase have been updated to match the new signature.

302-317: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [302-326]

The ParticleToMesh class has been updated to include a new template parameter Func for the function to calculate the deposit. This change allows for more flexibility in the calculations performed by the class. Ensure that all calls to this class throughout the codebase have been updated to match the new signature.

333-348: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [333-360]

The ParticleToMesh class has been updated to include a new template parameter Func for the function to calculate the deposit. This change allows for more flexibility in the calculations performed by the class. Ensure that all calls to this class throughout the codebase have been updated to match the new signature.

474-505: The operator() method in the Interpolator class has been updated to take a TensorField parameter for the momentum tensor. This change allows for the calculation of the momentum tensor during interpolation. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

564-606: A new MomentumTensorInterpolator class has been added as a derived class of Interpolator. This class includes a new operator() method to calculate the momentum tensor. This is a significant addition that enhances the functionality of the Interpolator class. Ensure that this new class is being used correctly throughout the codebase.

src/diagnostic/detail/types/fluid.hpp (9)

34-38: The dimension and interp_order constants are introduced. Ensure that these constants are used consistently throughout the codebase.

46-46: The compute method is introduced to compute the momentum tensor for ions. Ensure that this method is called appropriately in the codebase.

62-67: The isActiveDiag method is introduced to check if a diagnostic is active. Ensure that this method is used consistently throughout the codebase.

74-110: The compute method is implemented. It uses the MomentumTensorInterpolator to compute the momentum tensor for each ion population that requires it. Ensure that the computations are correct and that the results are used appropriately.

116-126: The createFiles method is updated to handle the momentum tensor diagnostic. Ensure that the files are created correctly and that they are used appropriately.

155-193: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [132-186]

The getDataSetInfo method is updated to handle the momentum tensor diagnostic. Ensure that the dataset information is retrieved correctly and that it is used appropriately.

231-251: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [192-251]

The initDataSets method is updated to handle the momentum tensor diagnostic. Ensure that the datasets are initialized correctly and that they are used appropriately.

269-290: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [258-288]

The write method is updated to handle the momentum tensor diagnostic. Ensure that the data is written correctly and that it is used appropriately.

308-314: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [307-314]

The writeAttributes method is updated to handle the momentum tensor diagnostic. Ensure that the attributes are written correctly and that they are used appropriately.

src/diagnostic/detail/h5writer.hpp (10)

23-48: The class H5Writer has been templated with ModelView. Ensure that all instances of H5Writer have been updated to use the new template parameter.

54-64: The constructor of H5Writer has been updated to accept additional parameters. Ensure that all instances of H5Writer have been updated to use the new constructor.

82-86: The method getDiagnosticWriterForType has been added. This method retrieves a diagnostic writer for a given type. Ensure that this method is used correctly throughout the codebase.

142-148: The method writeVecFieldAsDataset has been added. This method writes a vector field as a dataset. Ensure that this method is used correctly throughout the codebase.

162-168: The typeWriters_ unordered map has been updated to include additional diagnostic writers. Ensure that these new writers are used correctly throughout the codebase.

182-185: The copy and move constructors and assignment operators for H5Writer have been deleted. This is a good practice to prevent shallow copying of the object. However, ensure that there are no attempts to copy or move H5Writer objects in the codebase.

210-215: The dump method has been updated to include additional operations. Ensure that these changes do not affect the expected behavior of the method.

228-234: The dump method has been updated to finalize each diagnostic writer and update the file flags. Ensure that these changes do not affect the expected behavior of the method.

255-268: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [257-278]

The initializeDatasets_ method has been added. This method initializes datasets for each diagnostic. Ensure that this method is used correctly throughout the codebase.

296-301: The writeDatasets_ method has been added. This method writes datasets for each diagnostic. Ensure that this method is used correctly throughout the codebase.

tests/core/numerics/ion_updater/test_updater.cpp (6)

241-258: The addition of momentum tensor fields for different species is a good step towards the computation of pressure. Ensure that these fields are initialized and updated correctly in the simulation.

298-334: The initialization of the momentum tensor fields with the correct size is crucial. Make sure that the layout.allocSize function returns the correct size for each field.

381-418: The copy constructor seems to be correctly implemented, copying the momentum tensor fields from the source object. Ensure that this doesn't lead to any unintended side effects, such as aliasing issues.

467-473: Setting the buffer for the momentum tensor fields is a crucial step. Ensure that the buffers are correctly linked to the fields.

482-487: Setting the buffer for the momentum tensor fields of the proton population is correctly implemented. Ensure that the buffers are correctly linked to the fields.

496-501: Setting the buffer for the momentum tensor fields of the alpha population is correctly implemented. Ensure that the buffers are correctly linked to the fields.

src/core/data/tensorfield/tensorfield.hpp (1)

1-290: The TensorField class template is well-structured and follows good practices. It provides a clear interface for manipulating tensor fields and includes appropriate error handling. The use of constexpr and template metaprogramming for compile-time calculations is commendable. However, there are a few areas that could be improved for better maintainability and readability.

tests/core/data/ions/test_ions.cpp (1)

34-44: The changes in this hunk are consistent with the pull request summary. The SymTensorField1D type is introduced and used as a template parameter for IonPopulation1D. The InitFunctionT type is updated to use PHARE::initializer::InitFunction<1>.

src/restarts/detail/h5writer.hpp

src/amr/data/field/coarsening/field_coarsen_operator.hpp

src/core/data/vecfield/vecfield_component.hpp

src/core/data/tensorfield/tensorfield.hpp

coderabbitai

Review Status

Actionable comments generated: 5

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 32ae294 and fcfb6da.

Files selected for processing (27)

src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 hunks)
src/amr/physical_models/hybrid_model.hpp (1 hunks)
src/core/CMakeLists.txt (1 hunks)
src/core/data/grid/gridlayoutdefs.hpp (1 hunks)
src/core/data/grid/gridlayoutimplyee.hpp (4 hunks)
src/core/data/ions/ion_population/ion_population.hpp (8 hunks)
src/core/data/ions/ions.hpp (9 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield.hpp (2 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/core/hybrid/hybrid_quantities.hpp (2 hunks)
src/core/numerics/interpolator/interpolator.hpp (10 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (2 hunks)
src/diagnostic/detail/types/fluid.hpp (11 hunks)
src/diagnostic/detail/types/particle.hpp (1 hunks)
src/diagnostic/diagnostics.hpp (1 hunks)
src/phare_core.hpp (3 hunks)
src/python3/patch_level.hpp (6 hunks)
src/restarts/detail/h5writer.hpp (1 hunks)
tests/amr/models/test_models.cpp (3 hunks)
tests/amr/resources_manager/test_resources_manager.cpp (3 hunks)
tests/core/data/electrons/test_electrons.cpp (6 hunks)
tests/core/data/ion_population/test_ion_population.cpp (3 hunks)
tests/core/data/ions/test_ions.cpp (2 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
tests/diagnostic/test_diagnostics.hpp (2 hunks)

Files not summarized due to errors (12)

src/core/CMakeLists.txt: Error: Server error. Please try again later.
src/core/data/ions/ions.hpp: Error: Server error. Please try again later.
tests/diagnostic/test_diagnostics.hpp: Error: Server error. Please try again later.
src/core/data/vecfield/vecfield.hpp: Error: Server error. Please try again later.
src/core/data/grid/gridlayoutimplyee.hpp: Error: Server error. Please try again later.
src/diagnostic/detail/types/fluid.hpp: Error: Server error. Please try again later.
tests/core/data/electrons/test_electrons.cpp: Error: Server error. Please try again later.
tests/core/numerics/ion_updater/test_updater.cpp: Error: Server error. Please try again later.
src/core/data/grid/gridlayoutdefs.hpp: Error: Server error. Please try again later.
src/diagnostic/detail/types/electromag.hpp: Error: Server error. Please try again later.
src/core/hybrid/hybrid_quantities.hpp: Error: Server error. Please try again later.
src/core/data/tensorfield/tensorfield.hpp: Error: Server error. Please try again later.

Files not reviewed due to errors (14)

src/core/data/tensorfield/tensorfield.hpp (Error: Server error. Please try again later.)
src/amr/data/field/coarsening/field_coarsen_operator.hpp (Error: Server error. Please try again later.)
src/diagnostic/detail/types/fluid.hpp (Error: Server error. Please try again later.)
src/core/data/grid/gridlayoutimplyee.hpp (Error: Server error. Please try again later.)
src/core/hybrid/hybrid_quantities.hpp (Error: Server error. Please try again later.)
src/core/data/ions/ions.hpp (Error: Server error. Please try again later.)
src/core/numerics/interpolator/interpolator.hpp (Error: Server error. Please try again later.)
src/python3/patch_level.hpp (Error: Server error. Please try again later.)
tests/amr/resources_manager/test_resources_manager.cpp (Error: Server error. Please try again later.)
src/restarts/detail/h5writer.hpp (Error: Server error. Please try again later.)
src/diagnostic/detail/h5writer.hpp (Error: Server error. Please try again later.)
src/core/data/ions/ion_population/ion_population.hpp (Error: Server error. Please try again later.)
tests/core/data/electrons/test_electrons.cpp (Error: Server error. Please try again later.)
tests/core/numerics/ion_updater/test_updater.cpp (Error: Server error. Please try again later.)

Files skipped from review due to trivial changes (1)

src/amr/physical_models/hybrid_model.hpp

Additional comments: 15

src/phare_core.hpp (1)

33-45: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [33-57]

The addition of SymTensorField_t and the modification of IonPopulation_t are consistent with the pull request summary. Ensure that these changes are propagated throughout the codebase where these types are used.

tests/core/data/ion_population/test_ion_population.cpp (2)

29-37: The DummyTensorField struct has been added. Ensure that the isUsable and isSettable methods are correctly implemented and used throughout the codebase.

60-61: The AnIonPopulation struct now uses DummyTensorField in its instantiation. Ensure that this change is reflected in all relevant parts of the codebase.

src/core/data/grid/gridlayoutdefs.hpp (1)

74-91: The new momentum tensor components and pressure have been added to the gridDataT struct. Ensure that these new components are being used correctly throughout the codebase.

tests/core/data/ions/test_ions.cpp (1)

34-43: The introduction of the SymTensorField1D type and its use in the IonPopulation1D type alias is a significant change. Ensure that all uses of IonPopulation1D throughout the codebase are compatible with this change.

src/diagnostic/diagnostics.hpp (1)

55-61: Ensure that the h5::H5Writer<ModelView_t> class has the same interface as the previous h5::Writer<ModelView_t>. If not, you may need to update the code where Writer_t is used. Also, verify that the h5::H5Writer<ModelView_t> class is included in the project and that it is compatible with the rest of the codebase.

tests/amr/models/test_models.cpp (2)

33-47: The introduction of SymTensorField1D and its integration into IonsPop1D, HybridModelT, and MHDModelT is consistent with the pull request summary. Ensure that the new SymTensorField1D class is correctly implemented and that its integration does not introduce any breaking changes.

7-13: The included headers seem to be relevant to the changes made in this file. However, ensure that all necessary headers are included, especially for the new SymTensorField1D class.

src/diagnostic/detail/types/particle.hpp (1)

4-11: The inclusion of "core/data/grid/gridlayout.hpp" seems to be unnecessary as it is not used in the code. The removal of "amr/data/particles/particles_data.hpp" is fine if it's not used anymore. Ensure that these changes do not affect other parts of the codebase.

tests/diagnostic/test_diagnostics.hpp (2)

36-41: The function checkVecField is looping over all components of the VecField and checking each one. This is a good practice as it ensures all components are validated.

45-51: The Hi5Diagnostic constructor is initializing several member variables. Ensure that the passed hierarchy, hybridModel, and out parameters are valid before using them to initialize the member variables.

src/core/data/vecfield/vecfield.hpp (2)

18-18: The VecField is now an alias for TensorField with rank 1. Ensure that all uses of VecField in the codebase are compatible with this change.

29-34: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [20-31]

The average function now operates on VecField objects. Ensure that all calls to this function have been updated to match the new signature.
src/core/data/vecfield/vecfield_component.hpp (1)
46-46: The tensComponentMap should map to the new tensor components, not the vector components.
- {"x", Component::X}, {"y", Component::Y}, {"z", Component::Z}
+ {"xx", Component::XX}, {"xy", Component::XY}, {"xz", Component::XZ}, {"yy", Component::YY}, {"yz", Component::YZ}, {"zz", Component::ZZ}
src/core/CMakeLists.txt (1)

20-26: The new files related to TensorField and VecField have been added correctly to the list of source files. Ensure that these files exist in the correct directory and that they are included where necessary.

tests/core/data/ions/test_ions.cpp

src/core/data/vecfield/vecfield.hpp

coderabbitai

Review Status

Actionable comments generated: 38

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between fcfb6da and 15d97e9.

Files selected for processing (13)

pyphare/pyphare/pharein/init.py (9 hunks)
pyphare/pyphare/pharein/diagnostics.py (5 hunks)
pyphare/pyphare/pharein/simulation.py (18 hunks)
pyphare/pyphare/pharesee/hierarchy.py (2 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
tests/diagnostic/init.py (3 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (1 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Additional comments: 38

tests/simulator/test_initialization.py (1)

716-719: The change from iterating over values to key-value pairs in the dictionary doesn't seem to affect the logic as only diagInfo is used in the loop. Ensure that this change doesn't break any existing functionality.

src/diagnostic/detail/types/electromag.hpp (1)

151-157: The function writeTensorFieldAsDataset is being called instead of writeVecFieldAsDataset. Ensure that this change is intentional and that the function writeTensorFieldAsDataset exists and is correctly implemented.

tests/simulator/test_diagnostic_timestamps.py (2)

127-130: The change from ph.global_vars.sim.diagnostics to ph.global_vars.sim.diagnostics.items() is correct if diagnostics is a dictionary and you want to iterate over its items (key-value pairs). Ensure that diagname and diagInfo are used correctly within the loop.

169-172: Similar to the previous comment, the change from simulation.diagnostics to simulation.diagnostics.items() is correct if diagnostics is a dictionary. Ensure that diagname and diagInfo are used correctly within the loop.
pyphare/pyphare/pharesee/hierarchy.py (2)
171-178: The meshgrid function has been updated to handle a 3D case. Ensure that this change doesn't break any existing functionality that relies on this function. Also, consider adding a default case to handle unexpected ndim values.
+            if self.ndim == 3:
+                return np.meshgrid(self.x, self.y, self.z, indexing="ij")
+            else:
+                raise ValueError(f"Unexpected ndim value: {self.ndim}")
1499-1504: New quantities related to momentum tensor have been added to the isFieldQty function. Ensure that these new quantities are handled correctly in all places where isFieldQty is used.
tests/simulator/test_tagging.py (1)

182-185: Ensure that the path constructed from local_out and h5_filename_from(diagInfo) is valid and accessible.

tests/simulator/test_diagnostics.py (1)

191-194: Ensure that the change from list iteration to dictionary iteration doesn't affect the logic of the code. Verify that all instances of ph.global_vars.sim.diagnostics have been updated to reflect this change.

tests/diagnostic/__init__.py (2)

26-32: The new quantity "pressure_tensor" has been added to the list of quantities. Ensure that the FluidDiagnostics class and any related classes or functions can handle this new quantity correctly.

36-54: Ensure that the FluidDiagnostics and ParticleDiagnostics classes can handle the quantities "density", "flux", "domain", "levelGhost", and "patchGhost" correctly for each population in pops.

src/diagnostic/detail/types/fluid.hpp (7)

34-39: The use of static constexpr for dimension and interp_order is good for performance as these values are determined at compile time.

46-46: The compute function is introduced to compute the momentum tensor for each population that requires it. Ensure that this function is called appropriately in the codebase.

142-152: The createFiles function is updated to handle the momentum tensor. Ensure that the files are created correctly and that the momentum tensor data is written correctly.

181-219: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [158-212]

The getDataSetInfo function is updated to handle the momentum tensor. Ensure that the dataset information for the momentum tensor is retrieved correctly.

261-281: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [218-281]

The initDataSets function is updated to handle the momentum tensor. Ensure that the datasets for the momentum tensor are initialized correctly.

289-320: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [288-318]

The write function is updated to handle the momentum tensor. Ensure that the momentum tensor data is written correctly.

338-344: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [334-344]

The writeAttributes function is updated to handle the momentum tensor. Ensure that the attributes for the momentum tensor are written correctly.

src/diagnostic/detail/h5writer.hpp (13)

23-48: The class H5Writer has been introduced with a template parameter ModelView. It has several type aliases and static constants defined. The constructor and destructor are also defined. The class has a few public methods and a private member modelView_.

54-64: The constructor of H5Writer has been updated to accept four parameters. The destructor is also defined.

82-86: The method getDiagnosticWriterForType has been introduced which returns a writer for a given type.

163-169: The member variable writers has been renamed to typeWriters_ and its type has been updated.

183-186: The copy and move constructors and assignment operators have been deleted to prevent copying and moving of H5Writer objects.

211-216: The dump method has been updated to accept a vector of DiagnosticProperties pointers and a timestamp. It sets the timestamp and several attributes.

229-235: The dump method has been updated to finalize the writers and update the file flags.

239-244: The dump_level method has been introduced which dumps diagnostics for a specific level.

259-266: The initializeDatasets_ method has been introduced which initializes datasets for the given diagnostics.

273-279: The collectPatchAttributes lambda function has been introduced inside the initializeDatasets_ method. It collects patch attributes for the given diagnostics.

298-302: The writeDatasets_ method has been introduced which writes datasets for the given diagnostics.

309-314: The writePatch lambda function has been introduced inside the writeDatasets_ method. It writes a patch for the given diagnostics.

322-327: The writeDatasets_ method has been updated to write attributes for the given diagnostics.

pyphare/pyphare/pharein/__init__.py (4)

76-82: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [76-90]

The py_fn_wrapper class is used to wrap a function and convert its arguments and return values to numpy arrays. This is a good use of a class to encapsulate this behavior.

92-104: The fn_wrapper class inherits from py_fn_wrapper and overrides the __call__ method to convert the numpy array to a C++ SubSpan. This is a good use of inheritance and method overriding.

106-114: The clearDict function calls pp.stop(). It's not clear what this does without more context. Ensure that this is the correct and safe way to clear the dictionary.

348-355: This block of code is used to handle electrons. It iterates over a dictionary and adds each item to the simulation. This is a good use of a for loop to iterate over a dictionary.

pyphare/pyphare/pharein/simulation.py (4)

151-152: The code only allows interp_order to be 1, 2, or 3. If there are other valid values for interp_order in your context, consider updating this check.

162-163: The code only allows particle_pusher to be modified_boris. If there are other valid values for particle_pusher in your context, consider updating this check.

172-173: The code only allows layout to be yee. If there are other valid values for layout in your context, consider updating this check.

222-246: The code checks if refined_particle_nbr is in the valid list for the given ndim and interp. If there are other valid values for refined_particle_nbr in your context, consider updating this check.

tests/simulator/test_diagnostics.py

pyphare/pyphare/pharein/diagnostics.py

src/diagnostic/detail/h5writer.hpp

pyphare/pyphare/pharein/simulation.py

coderabbitai · 2023-11-09T15:25:40Z

pyphare/pyphare/pharein/simulation.py

    def wrapper(simulation_object, **kwargs):
-        accepted_keywords = ['domain_size', 'cells', 'dl', 'particle_pusher', 'final_time',
-                             'time_step', 'time_step_nbr', 'layout', 'interp_order', 'origin',
-                             'boundary_types', 'refined_particle_nbr', 'path', 'nesting_buffer',
-                             'diag_export_format', 'refinement_boxes', 'refinement', 'clustering',
-                             'smallest_patch_size', 'largest_patch_size', "diag_options",
-                             'resistivity', 'hyper_resistivity', 'strict', "restart_options", 'tag_buffer', ]
+        accepted_keywords = [
+            "domain_size",
+            "cells",
+            "dl",
+            "particle_pusher",
+            "final_time",
+            "time_step",
+            "time_step_nbr",
+            "layout",
+            "interp_order",
+            "origin",
+            "boundary_types",
+            "refined_particle_nbr",
+            "path",
+            "nesting_buffer",
+            "diag_export_format",
+            "refinement_boxes",
+            "refinement",
+            "clustering",
+            "smallest_patch_size",
+            "largest_patch_size",
+            "diag_options",
+            "resistivity",
+            "hyper_resistivity",
+            "strict",
+            "restart_options",
+            "tag_buffer",
+        ]


The function checker wraps another function and checks if the keys in kwargs are in the list of accepted keywords. However, it doesn't handle the case where kwargs is not a dictionary. Consider adding a check for this case.

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 15d97e9 and 5a003d5.

Files selected for processing (1)

src/core/data/vecfield/vecfield_component.hpp (1 hunks)

Additional comments: 3

src/core/data/vecfield/vecfield_component.hpp (3)

12-12: New tensor components have been added to the Component enum class. Ensure that these new components are handled correctly in all places where this enum is used.

35-41: The check function has been updated to include the new tensor components. Ensure that this function is used correctly throughout the codebase.

43-47: The vecComponentMap and tensComponentMap have been defined correctly. Ensure that these maps are used correctly throughout the codebase.

coderabbitai

Review Status

Actionable comments generated: 20

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between fcfb6da and f23733f.

Files selected for processing (14)

pyphare/pyphare/pharein/init.py (9 hunks)
pyphare/pyphare/pharein/diagnostics.py (5 hunks)
pyphare/pyphare/pharein/simulation.py (18 hunks)
pyphare/pyphare/pharesee/hierarchy.py (2 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
tests/diagnostic/init.py (3 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (1 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Additional comments: 49

tests/simulator/test_diagnostic_timestamps.py (2)

127-130: The change from ph.global_vars.sim.diagnostics to ph.global_vars.sim.diagnostics.items() indicates that diagnostics is now a dictionary or similar key-value data structure. Ensure that the keys and values of this dictionary are being used correctly in the loop.

169-172: Similar to the previous comment, ensure that the keys and values of simulation.diagnostics.items() are being used correctly in the loop.

pyphare/pyphare/pharesee/hierarchy.py (2)

171-178: The meshgrid function has been updated to handle different dimensions and a select parameter has been added. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

1499-1504: New momentum tensor components have been added to the list of field quantities. Ensure that these new components are being handled correctly in all relevant parts of the codebase.

src/diagnostic/detail/types/electromag.hpp (3)

69-75: The previous comment about the lambda function infoVF capturing all variables from the outer scope by reference is still valid. Consider capturing only the necessary ones to improve readability and maintainability.

104-110: The previous comment about the lambda function initVF capturing all variables from the outer scope by reference is still valid. Consider capturing only the necessary ones.

151-157: The function h5Writer.writeVecFieldAsDataset has been replaced with h5Writer.writeTensorFieldAsDataset. Ensure that this change is reflected wherever this function is called and that the new function writeTensorFieldAsDataset is implemented correctly.

src/core/data/vecfield/vecfield_component.hpp (3)

16-25: The componentMap function now returns different maps based on the rank. This is a good approach to separate the logic for vector and tensor components. However, it throws a runtime error for ranks other than 1 or 2. Consider handling this case more gracefully or ensuring that the function is never called with an invalid rank.

35-41: The check function has been updated to include the new tensor components. This is a good update to ensure that the function works correctly with the new components.

43-47: The vecComponentMap and tensComponentMap have been defined correctly and include all the necessary components. This is a good approach to separate the components for vectors and tensors.
tests/simulator/test_diagnostics.py (1)
182-187: The previous comment about removing the list comprehension inside any() is still valid. The updated code should look like this:
- any(
-     [
-         diagInfo.quantity.endswith("domain")
-         for diagname, diagInfo in ph.global_vars.sim.diagnostics.items()
-     ]
- )
+ any(diagInfo.quantity.endswith("domain") for diagname, diagInfo in ph.global_vars.sim.diagnostics.items())
src/diagnostic/detail/types/fluid.hpp (6)

34-39: The use of static constexpr for dimension and interp_order is good for performance as these values are determined at compile time.

144-152: The createFiles function checks if files need to be created for each population and for the total ions. This is a good practice as it ensures that files are only created when necessary.

181-219: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [163-212]

The getDataSetInfo function retrieves the dataset information for each population and for the total ions. This is a good practice as it ensures that the dataset information is always up-to-date.

261-281: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [219-281]

The initDataSets function initializes the datasets for each population and for the total ions. This is a good practice as it ensures that the datasets are always initialized correctly.

289-318: The write function writes the data for each population and for the total ions to the respective datasets. This is a good practice as it ensures that the data is always written correctly.

338-344: The writeAttributes function writes the attributes for each population and for the total ions. This is a good practice as it ensures that the attributes are always written correctly.

tests/diagnostic/__init__.py (2)

29-32: The addition of "pressure_tensor" to the list of quantities is consistent with the pull request summary. Ensure that the "pressure_tensor" quantity is correctly handled in the FluidDiagnostics function.

47-54: The addition of commas after the "population_name" parameter in the FluidDiagnostics and ParticleDiagnostics function calls is a minor change that improves readability and is consistent with Python's style guide (PEP 8).

src/diagnostic/detail/h5writer.hpp (8)

23-48: The class H5Writer has been introduced as a template class with ModelView as the template parameter. It has several type aliases and static constants defined. The constructor and destructor are also defined.

54-64: The constructor of H5Writer has been updated to accept additional parameters and initialize member variables accordingly. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

142-149: The method writeVecFieldAsDataset has been renamed to writeTensorFieldAsDataset and updated to handle tensor fields. This change is in line with the introduction of tensor fields in the codebase.

163-169: The member variable writers has been renamed to typeWriters_ and its type has been changed from std::unordered_map<std::string, Writer> to std::unordered_map<std::string, std::shared_ptr<H5TypeWriter<This>>>. This change is in line with the renaming of the Writer class to H5Writer.

183-186: The copy and move constructors and assignment operators for H5Writer have been explicitly deleted to prevent shallow copying. This is a good practice when the class manages resources that should not be shared between instances.

229-244: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [211-236]

The dump and dump_level member functions have been moved to the H5Writer class from the Writer class. Ensure that all calls to these functions throughout the codebase have been updated to match the new class.

256-269: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [259-294]

The private member functions initializeDatasets_ and writeDatasets_ have been introduced. These functions are used to initialize and write datasets respectively.

309-315: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [298-327]

The writeDatasets_ function has been updated to handle tensor fields. This change is in line with the introduction of tensor fields in the codebase.

pyphare/pyphare/pharein/diagnostics.py (6)

100-100: The error message could be more specific by stating that a Simulation object must be created before adding diagnostics.

127-130: The error message could be more specific by stating that flush_every cannot be negative.

183-186: The error message could be more specific by stating that the provided quantity is not a valid electromag diagnostics.

304-308: The error message could be more specific by stating that the provided quantity is not a valid particle diagnostics.

330-333: The error message could be more specific by stating that the 'extent' parameter is missing which is required by 'space_box' the ParticleDiagnostics type.

367-371: The error message could be more specific by stating that the provided quantity is not a valid meta diagnostics.

pyphare/pyphare/pharein/simulation.py (15)

108-142: This block of code checks for different combinations of keys in kwargs and performs calculations accordingly. It's important to ensure that the keys are being passed correctly in all function calls.

149-154: The function check_interp_order checks if the interpolation order is within the valid range [1,2,3]. If not, it raises a ValueError. This is a good practice for input validation.

161-164: The function check_pusher checks if the pusher is "modified_boris". If not, it raises a ValueError. This is a good practice for input validation.

171-174: The function check_layout checks if the layout is "yee". If not, it raises a ValueError. This is a good practice for input validation.

105-201: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [189-211]

The function check_boundaries checks if the boundary types are valid and if the length of boundary types matches the number of dimensions. If not, it raises a ValueError. This is a good practice for input validation.

222-248: The function check_refined_particle_nbr checks if the refined particle number is valid for the given dimensions and interpolation order. If not, it raises a ValueError. This is a good practice for input validation.

255-256: The function check_origin returns the origin, which is either provided in kwargs or defaults to [0.0] * ndim. This seems to be a straightforward getter function.

272-308: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [264-292]

The function as_list_per_level accepts various formats of boxes and returns a dictionary with level keys and a list of boxes. This function seems to be handling different input formats well.

316-335: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [296-354]

The function check_refinement_boxes checks if the refinement boxes are valid and compatible with the smallest patch size, and raises a ValueError if not. This is a good practice for input validation.

340-449: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [362-541]

The function check_patch_size checks if the patch sizes are valid and compatible with the number of domain cells, and raises a ValueError if not. This is a good practice for input validation.

546-551: The function check_optional_keywords checks the refinement type and adds "max_nbr_levels" to the list of keywords if the refinement type is not "boxes". This seems to be a straightforward getter function.

518-556: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [553-557]

The function check_resistivity checks if the resistivity is non-negative, and raises a ValueError if not. This is a good practice for input validation.

562-567: The function check_hyper_resistivity checks if the hyper resistivity is non-negative, and raises a ValueError if not. This is a good practice for input validation.

571-577: The function check_clustering checks if the clustering type is valid, and raises a ValueError if not. This is a good practice for input validation.

647-672: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [622-666]

This block of code checks various parameters and raises a ValueError if any of them are invalid. This is a good practice for input validation.

tests/simulator/test_initialization.py (1)

716-719: The change in the loop structure seems fine. However, ensure that ph.global_vars.sim.diagnostics is a dictionary and that each diagInfo has a quantity attribute that can be checked for the "patchGhost" suffix.

coderabbitai · 2023-11-09T17:16:00Z

tests/simulator/test_tagging.py

                any(
                    [
                        diagInfo.quantity.endswith("tags")
-                        for diagInfo in ph.global_vars.sim.diagnostics
+                        for diagname, diagInfo in ph.global_vars.sim.diagnostics.items()
                    ]
                )
            )


The use of any() with a list comprehension is unnecessary. It can be replaced with a generator expression, which is more efficient as it stops iterating as soon as the condition is met.

- any([diagInfo.quantity.endswith("tags") for diagname, diagInfo in ph.global_vars.sim.diagnostics.items()]) + any(diagInfo.quantity.endswith("tags") for diagname, diagInfo in ph.global_vars.sim.diagnostics.items())

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.

Suggested change

any(

[

diagInfo.quantity.endswith("tags")

for diagInfo in ph.global_vars.sim.diagnostics

for diagname, diagInfo in ph.global_vars.sim.diagnostics.items()

]

)

)

any(

diagInfo.quantity.endswith("tags")

for diagname, diagInfo in ph.global_vars.sim.diagnostics.items()

)

coderabbitai · 2023-11-09T17:16:00Z

tests/simulator/test_diagnostics.py

+            for diagname, diagInfo in ph.global_vars.sim.diagnostics.items():
                h5_filepath = os.path.join(local_out, h5_filename_from(diagInfo))
                self.assertTrue(os.path.exists(h5_filepath))



Ensure that the file path is correctly formed and the file exists before opening it. This will prevent potential FileNotFoundError.

h5_filepath = os.path.join(local_out, h5_filename_from(diagInfo)) if not os.path.exists(h5_filepath): raise FileNotFoundError(f"No file found at {h5_filepath}")

coderabbitai · 2023-11-09T17:16:02Z

src/core/data/tensorfield/tensorfield.hpp

+        if constexpr (N == 6)
+        {
+            return {{name_ + "_xx", name_ + "_xy", name_ + "_xz", name_ + "_yy", name_ + "_yz",
+                     name_ + "_zz"}};
+        }
+        else if constexpr (N == 3)
+        {
+            return {{name_ + "_x", name_ + "_y", name_ + "_z"}};
+        }
+    }


The makeNames_ function could be simplified by using a single return statement with a conditional operator. This would make the code more concise and easier to read.

std::array<std::string, N> makeNames_() { return (N == 6) ? std::array{name_ + "_xx", name_ + "_xy", name_ + "_xz", name_ + "_yy", name_ + "_yz", name_ + "_zz"} : std::array{name_ + "_x", name_ + "_y", name_ + "_z"}; }

coderabbitai · 2023-11-09T17:16:02Z

pyphare/pyphare/pharein/__init__.py

-        addInitFunction(partinit_path+"density", fn_wrapper(d["density"]))
-        addInitFunction(partinit_path+"bulk_velocity_x", fn_wrapper(d["vx"]))
-        addInitFunction(partinit_path+"bulk_velocity_y", fn_wrapper(d["vy"]))
-        addInitFunction(partinit_path+"bulk_velocity_z", fn_wrapper(d["vz"]))
-        addInitFunction(partinit_path+"thermal_velocity_x",fn_wrapper(d["vthx"]))
-        addInitFunction(partinit_path+"thermal_velocity_y",fn_wrapper(d["vthy"]))
-        addInitFunction(partinit_path+"thermal_velocity_z",fn_wrapper(d["vthz"]))
-        add_int(partinit_path+"nbr_part_per_cell", d["nbrParticlesPerCell"])
-        add_double(partinit_path+"charge", d["charge"])
-        add_string(partinit_path+"basis", "cartesian")
+        add_string(pop_path + "{:d}/name".format(pop_index), pop)
+        add_double(pop_path + "{:d}/mass".format(pop_index), d["mass"])
+        add_string(partinit_path + "name", "maxwellian")
+
+        addInitFunction(partinit_path + "density", fn_wrapper(d["density"]))
+        addInitFunction(partinit_path + "bulk_velocity_x", fn_wrapper(d["vx"]))
+        addInitFunction(partinit_path + "bulk_velocity_y", fn_wrapper(d["vy"]))
+        addInitFunction(partinit_path + "bulk_velocity_z", fn_wrapper(d["vz"]))
+        addInitFunction(partinit_path + "thermal_velocity_x", fn_wrapper(d["vthx"]))
+        addInitFunction(partinit_path + "thermal_velocity_y", fn_wrapper(d["vthy"]))
+        addInitFunction(partinit_path + "thermal_velocity_z", fn_wrapper(d["vthz"]))
+        add_int(partinit_path + "nbr_part_per_cell", d["nbrParticlesPerCell"])
+        add_double(partinit_path + "charge", d["charge"])
+        add_string(partinit_path + "basis", "cartesian")
        if "init" in d and "seed" in d["init"]:
-            pp.add_optional_size_t(partinit_path+"init/seed", d["init"]["seed"])
+            pp.add_optional_size_t(partinit_path + "init/seed", d["init"]["seed"])

    add_string("simulation/electromag/name", "EM")
    add_string("simulation/electromag/electric/name", "E")

-
    add_string("simulation/electromag/magnetic/name", "B")
    maginit_path = "simulation/electromag/magnetic/initializer/"
-    addInitFunction(maginit_path+"x_component", fn_wrapper(modelDict["bx"]))
-    addInitFunction(maginit_path+"y_component", fn_wrapper(modelDict["by"]))
-    addInitFunction(maginit_path+"z_component", fn_wrapper(modelDict["bz"]))
-
+    addInitFunction(maginit_path + "x_component", fn_wrapper(modelDict["bx"]))
+    addInitFunction(maginit_path + "y_component", fn_wrapper(modelDict["by"]))
+    addInitFunction(maginit_path + "z_component", fn_wrapper(modelDict["bz"]))

    #### adding diagnostics
    diag_path = "simulation/diagnostics/"
-    for diag in simulation.diagnostics:
-        type_path = diag_path + diag.type + '/'
+    for _, diag in simulation.diagnostics.items():
+        print(f"DEBUGGGGG  {_}, write_timestamps : {diag.write_timestamps}")
+        type_path = diag_path + diag.type + "/"
        name_path = type_path + diag.name
-        add_string(name_path + "/" + 'type' , diag.type)
-        add_string(name_path + "/" + 'quantity' , diag.quantity)
+        add_string(name_path + "/" + "type", diag.type)
+        add_string(name_path + "/" + "quantity", diag.quantity)
        add_size_t(name_path + "/" + "flush_every", diag.flush_every)
-        pp.add_array_as_vector(name_path + "/" + "write_timestamps", diag.write_timestamps)
-        pp.add_array_as_vector(name_path + "/" + "compute_timestamps", diag.compute_timestamps)
-        add_size_t(name_path + "/" + 'n_attributes' , len(diag.attributes))
+        pp.add_array_as_vector(
+            name_path + "/" + "write_timestamps", diag.write_timestamps
+        )
+        pp.add_array_as_vector(
+            name_path + "/" + "compute_timestamps", diag.compute_timestamps
+        )
+        add_size_t(name_path + "/" + "n_attributes", len(diag.attributes))
        for attr_idx, attr_key in enumerate(diag.attributes):
-            add_string(name_path + "/" + f'attribute_{attr_idx}_key' , attr_key)
-            add_string(name_path + "/" + f'attribute_{attr_idx}_value' , diag.attributes[attr_key])
+            add_string(name_path + "/" + f"attribute_{attr_idx}_key", attr_key)
+            add_string(
+                name_path + "/" + f"attribute_{attr_idx}_value",
+                diag.attributes[attr_key],
+            )

    if len(simulation.diagnostics) > 0:
        if simulation.diag_options is not None and "options" in simulation.diag_options:
-            add_string(diag_path + "filePath", simulation.diag_options["options"]["dir"])
+            add_string(
+                diag_path + "filePath", simulation.diag_options["options"]["dir"]
+            )
            if "mode" in simulation.diag_options["options"]:
-                add_string(diag_path + "mode", simulation.diag_options["options"]["mode"])
+                add_string(
+                    diag_path + "mode", simulation.diag_options["options"]["mode"]
+                )
            if "fine_dump_lvl_max" in simulation.diag_options["options"]:
-                add_int(diag_path + "fine_dump_lvl_max", simulation.diag_options["options"]["fine_dump_lvl_max"])
+                add_int(
+                    diag_path + "fine_dump_lvl_max",
+                    simulation.diag_options["options"]["fine_dump_lvl_max"],
+                )
        else:
            add_string(diag_path + "filePath", "phare_output")
    #### diagnostics added

-
-
    #### adding restarts
    if simulation.restart_options is not None:



The populateDict function is quite long and does a lot of different things. Consider breaking it up into smaller, more manageable functions. This will make the code easier to understand and maintain.

coderabbitai

Review Status

Actionable comments generated: 36

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between fcfb6da and da875f4.

Files selected for processing (15)

pyphare/pyphare/core/gridlayout.py (3 hunks)
pyphare/pyphare/pharein/init.py (9 hunks)
pyphare/pyphare/pharein/diagnostics.py (5 hunks)
pyphare/pyphare/pharein/simulation.py (18 hunks)
pyphare/pyphare/pharesee/hierarchy.py (3 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
tests/diagnostic/init.py (3 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (2 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Additional comments: 46

tests/simulator/test_diagnostic_timestamps.py (2)

124-130: The change from directly iterating over ph.global_vars.sim.diagnostics to ph.global_vars.sim.diagnostics.items() is correct and necessary for Python 3, as items() returns a view object that displays a list of a dictionary's key, value tuple pair.

166-172: The same applies here for simulation.diagnostics.items(). This change is correct and necessary for Python 3.
tests/simulator/test_initialization.py (1)
716-716: The change in the loop structure seems to be correct and more Pythonic. However, ensure that diagname and diagInfo are used correctly within the loop, and that all instances of accessing diagnostic information have been updated accordingly.
for diagname, diagInfo in ph.global_vars.sim.diagnostics.items()
pyphare/pyphare/pharesee/hierarchy.py (3)

171-178: The meshgrid function has been updated to handle different dimensions. Ensure that all calls to this function throughout the codebase have been updated to match the new behavior.

1165-1170: New momentum tensor components have been added to the mapping of field quantities. Ensure that these new components are being used correctly throughout the codebase.

1505-1510: New momentum tensor components have been added to the list of exported functions. Ensure that these new components are being used correctly throughout the codebase.

pyphare/pyphare/core/gridlayout.py (3)

31-40: The new properties "Mxx", "Mxy", "Mxz", "Myy", "Myz", and "Mzz" have been added with the value "primal". Ensure that these new properties are being used correctly throughout the codebase.

60-69: The new properties "Mxx", "Mxy", "Mxz", "Myy", "Myz", and "Mzz" have been added with the value "primal". Ensure that these new properties are being used correctly throughout the codebase.

89-98: The new properties "Mxx", "Mxy", "Mxz", "Myy", "Myz", and "Mzz" have been added with the value "primal". Ensure that these new properties are being used correctly throughout the codebase.

src/core/data/vecfield/vecfield_component.hpp (2)

35-41: The check function has been updated to include the new tensor components. Ensure that all calls to this function throughout the codebase have been updated to handle the new components.

43-47: The component maps for vectors and tensors have been separated. This is a good approach to separate the logic for vector and tensor components.

src/diagnostic/detail/types/electromag.hpp (1)

151-157: The function h5Writer.writeTensorFieldAsDataset is being used instead of h5Writer.writeVecFieldAsDataset. Ensure that this change is intentional and that the writeTensorFieldAsDataset function is capable of handling all the data types and structures that writeVecFieldAsDataset was previously handling.
tests/simulator/test_tagging.py (2)
172-178: The previous comment is still valid. The list comprehension can be replaced with a generator expression for efficiency.
- any([diagInfo.quantity.endswith("tags") for diagname, diagInfo in ph.global_vars.sim.diagnostics.items()])
+ any(diagInfo.quantity.endswith("tags") for diagname, diagInfo in ph.global_vars.sim.diagnostics.items())
182-185: The previous comment is still valid. It's important to ensure that the file path exists before opening it to avoid a potential FileNotFoundError.
if os.path.exists(h5_filepath):
    h5_file = h5py.File(h5_filepath, "r")
else:
    raise FileNotFoundError(f"No such file or directory: '{h5_filepath}'")
tests/simulator/test_diagnostics.py (3)
182-187: The previous comment about removing the list comprehension inside any() is still valid. The updated code would look like this:
any(diagInfo.quantity.endswith("domain") for diagname, diagInfo in ph.global_vars.sim.diagnostics.items())
191-194: The previous comment about checking if the file exists before opening it is still valid. The updated code would look like this:
h5_filepath = os.path.join(local_out, h5_filename_from(diagInfo))
if not os.path.exists(h5_filepath):
    raise FileNotFoundError(f"No file found at {h5_filepath}")
233-234: Ensure that setting self.simulator and ph.global_vars.sim to None does not cause any issues elsewhere in the code.
tests/diagnostic/__init__.py (2)

26-32: The ph.FluidDiagnostics call has been updated with additional parameters. Ensure that the function definition has been updated to handle these parameters.

36-54: The ph.FluidDiagnostics and ph.ParticleDiagnostics calls within the loops have been updated with additional parameters. Ensure that the function definitions have been updated to handle these parameters.

src/diagnostic/detail/types/fluid.hpp (8)

2-8: The new header file core/numerics/interpolator/interpolator.hpp is included. Ensure that this file exists and is accessible from this location.

34-49: The FluidDiagnosticWriter class has been updated to include a new member function compute. Ensure that this function is implemented correctly and used appropriately in the codebase.

58-68: A new private member function isActiveDiag is introduced. This function checks if a specific diagnostic is active based on the diagnostic quantity and the tree variable. This function seems to be used correctly in the codebase.

142-152: The createFiles function has been updated to handle the creation of files for the momentum tensor. Ensure that the files are created correctly and can be accessed and used as expected.

181-219: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [163-212]

The getDataSetInfo function has been updated to handle the momentum tensor. Ensure that the dataset information for the momentum tensor is retrieved correctly.

261-281: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [224-281]

The initDataSets function has been updated to handle the initialization of datasets for the momentum tensor. Ensure that the datasets are initialized correctly and can be accessed and used as expected.

289-318: The write function has been updated to handle the writing of the momentum tensor. Ensure that the momentum tensor is written correctly and can be accessed and used as expected.

338-344: The writeAttributes function has been updated to handle the writing of attributes for the momentum tensor. Ensure that the attributes are written correctly and can be accessed and used as expected.

src/core/data/tensorfield/tensorfield.hpp (11)

120-139: The previous comment about refactoring the getComponent method to reduce code duplication is still valid. The same applies to the getComponent method in lines 144-163.

256-265: The previous comment about simplifying the makeNames_ function with a single return statement and a conditional operator is still valid.

19-29: The dimFromRank function uses if constexpr to determine the dimension based on the rank. This is a good use of compile-time conditionals to optimize performance.

38-42: The use of type aliases and constexpr makes the code more readable and maintainable.

63-66: The getFieldNamesAndQuantities method uses std::make_index_sequence and makeResProp_ to generate an array of TensorFieldProperties. This is a good use of modern C++ features to generate compile-time sequences.

70-78: The setBuffer method checks if the buffer name exists in nameToIndex_ before setting the buffer. This is a good practice to prevent setting buffers for non-existent names.

84-94: The isUsable and isSettable methods use std::all_of to check if all components are usable or settable. This is a good use of STL algorithms to simplify code.

98-109: The zero method checks if the tensor field is usable before zeroing all components. This is a good practice to prevent operations on unusable tensor fields.

216-229: The copyData method checks if both the source and destination tensor fields are usable before copying data. This is a good practice to prevent operations on unusable tensor fields.

232-235: The begin, end, cbegin, and cend methods provide iterator access to the components. This is a good practice for providing STL-like access to the components.

268-274: The makeMap_ method uses std::make_index_sequence and fold expressions to generate a map of component names to indices. This is a good use of modern C++ features to generate compile-time sequences and maps.
src/diagnostic/detail/h5writer.hpp (8)
23-33: The class declarations for ElectromagDiagnosticWriter, FluidDiagnosticWriter, ParticlesDiagnosticWriter, and MetaDiagnosticWriter have been moved inside the H5Writer class. Ensure that all references to these classes in the codebase have been updated accordingly.

142-149: The method writeVecFieldAsDataset has been renamed to writeTensorFieldAsDataset and updated to handle tensor fields. Ensure that all calls to this method have been updated to match the new name and functionality.
-    template<typename VecField>
-    static void writeVecFieldAsDataset(HighFiveFile& h5, std::string path,
-                                       VecField& vecField)
+    template<typename TensorField>
+    static void writeTensorFieldAsDataset(HighFiveFile& h5, std::string path,
+                                          TensorField& tensorField)
163-169: The typeWriters_ unordered map has been updated with new diagnostic writer types. Ensure that all references to this map have been updated accordingly.

183-186: The copy and move constructors and assignment operators for H5Writer have been explicitly deleted to prevent shallow copying. This is a good practice for classes that manage resources.

211-215: The dump method has been updated to include a timestamp parameter. Ensure that all calls to this method have been updated to include this parameter.

229-235: The dump method has been updated to finalize each diagnostic writer and update the file flags. Ensure that this change does not introduce any unexpected behavior.

256-269: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [259-294]

The initializeDatasets_ method has been updated to create files for each diagnostic and get dataset information for each patch. Ensure that this change does not introduce any unexpected behavior.

309-315: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [298-327]

The writeDatasets_ method has been updated to write each diagnostic and write attributes for each diagnostic. Ensure that this change does not introduce any unexpected behavior.

coderabbitai · 2023-11-09T17:49:03Z

src/diagnostic/detail/types/electromag.hpp

    std::string lvlPatchID = std::to_string(iLevel) + "_" + patchID;

    auto infoVF = [&](auto& vecF, std::string name, auto& attr) {
-        for (auto& [id, type] : core::Components::componentMap)
+        for (auto& [id, type] : core::Components::componentMap())
        {
            // highfive doesn't accept uint32 which ndarray.shape() is
            auto const& array_shape = vecF.getComponent(type).shape();


The lambda function infoVF is capturing all variables from the outer scope by reference. If any of these variables are not used, consider capturing only the necessary ones to improve readability and maintainability.

coderabbitai · 2023-11-09T17:49:03Z

src/diagnostic/detail/types/electromag.hpp

    auto vecFields = h5Writer.modelView().getElectromagFields();

    auto initVF = [&](auto& path, auto& attr, std::string key, auto null) {
-        for (auto& [id, type] : core::Components::componentMap)
+        for (auto& [id, type] : core::Components::componentMap())
        {
            auto vFPath = path + "/" + key + "_" + id;
            h5Writer.template createDataSet<FloatType>(


Similar to the previous comment, the lambda function initVF is capturing all variables from the outer scope by reference. Consider capturing only the necessary ones.

coderabbitai · 2023-11-09T17:49:03Z

tests/diagnostic/__init__.py

 def timestamps_with_step(sim, dump_step):
    import numpy as np
+
    nbr_dump_step = sim.final_time / dump_step
    return dump_step * np.arange(nbr_dump_step)

+
 def all_timestamps(sim):
    import numpy as np
+
    nbr_dump_step = int(sim.final_time / sim.time_step) + 1
    return sim.time_step * np.arange(nbr_dump_step)



The import statement import numpy as np is repeated in both timestamps_with_step and all_timestamps functions. Consider moving it to the top of the file to follow the PEP 8 style guide.

+ import numpy as np + def timestamps_with_step(sim, dump_step): - import numpy as np ... def all_timestamps(sim): - import numpy as np ...

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.

Suggested change

def timestamps_with_step(sim, dump_step):

import numpy as np

nbr_dump_step = sim.final_time / dump_step

return dump_step * np.arange(nbr_dump_step)

def all_timestamps(sim):

import numpy as np

nbr_dump_step = int(sim.final_time / sim.time_step) + 1

return sim.time_step * np.arange(nbr_dump_step)

import numpy as np

def timestamps_with_step(sim, dump_step):

nbr_dump_step = sim.final_time / dump_step

return dump_step * np.arange(nbr_dump_step)

def all_timestamps(sim):

nbr_dump_step = int(sim.final_time / sim.time_step) + 1

return sim.time_step * np.arange(nbr_dump_step)

coderabbitai · 2023-11-09T17:49:03Z

src/diagnostic/detail/types/fluid.hpp

+    core::MomentumTensorInterpolator<dimension, interp_order> interpolator;
+
+    auto& h5Writer  = this->h5Writer_;
+    auto& modelView = h5Writer.modelView();
+    auto& ions      = modelView.getIons();
+    auto minLvl     = this->h5Writer_.minLevel;
+    auto maxLvl     = this->h5Writer_.maxLevel;
+    // compute the momentum tensor for each population that requires it
+    // compute for all ions but that requires the computation of all pop
+    std::string tree{"/ions/"};
+    if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
+    {
+        auto computeMomentumTensor
+            = [&](GridLayout& layout, std::string patchID, std::size_t iLvel) {
+                  for (auto& pop : ions)
+                  {
+                      std::string tree{"/ions/pop/" + pop.name() + "/"};
+                      auto& pop_momentum_tensor = pop.momentumTensor();
+                      auto domainParts          = core::makeIndexRange(pop.domainParticles());
+                      auto patchGhostParts      = core::makeIndexRange(pop.patchGhostParticles());
+
+                      // use levelGhostPartsNew because levelGhostParts may be empty
+                      // and diags are typically done at sync time where levelGhostPartsNew is
+                      // defined
+                      auto levelGhostParts = core::makeIndexRange(pop.levelGhostParticlesNew());
+
+                      interpolator(domainParts, pop_momentum_tensor, layout);
+                      interpolator(patchGhostParts, pop_momentum_tensor, layout);
+                      interpolator(levelGhostParts, pop_momentum_tensor, layout);
+                  }
+                  ions.computeFullMomentumTensor();
+              };
+        modelView.visitHierarchy(computeMomentumTensor, minLvl, maxLvl);
+    }
+    else // if not computing total momentum tensor, user may want to compute it for some pop
+    {
+        for (auto& pop : ions)
+        {
+            std::string tree{"/ions/pop/" + pop.name() + "/"};
+
+            auto computePopMomentumTensor
+                = [&](GridLayout& layout, std::string patchID, std::size_t iLvel) {
+                      auto& pop_momentum_tensor = pop.momentumTensor();
+                      auto domainParts          = core::makeIndexRange(pop.domainParticles());
+                      auto patchGhostParts      = core::makeIndexRange(pop.patchGhostParticles());
+
+                      // use levelGhostPartsNew because levelGhostParts may be empty
+                      // and diags are typically done at sync time where levelGhostPartsNew is
+                      // defined
+                      auto levelGhostParts = core::makeIndexRange(pop.levelGhostParticlesNew());
+
+                      interpolator(domainParts, pop_momentum_tensor, layout);
+                      interpolator(patchGhostParts, pop_momentum_tensor, layout);
+                      interpolator(levelGhostParts, pop_momentum_tensor, layout);
+                  };
+            if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
+            {
+                modelView.visitHierarchy(computePopMomentumTensor, minLvl, maxLvl);
+            }
+        }
+    }


The compute function is well-structured and handles the computation of the momentum tensor for each population. However, the function is quite long and complex. Consider breaking it down into smaller, more manageable functions to improve readability and maintainability.

coderabbitai · 2023-11-09T17:49:04Z

pyphare/pyphare/pharein/diagnostics.py

            # just take mandatory arguments from the dict
            # since if we arrived here we are sure they are there

-            kwargs['path'] = kwargs.get("path", './')
+            kwargs["path"] = kwargs.get("path", "./")

            return func(diagnostics_object, name, **kwargs)


The try block is not necessary here as no exception is being caught. The try block can be removed for simplicity.

coderabbitai · 2023-11-09T17:49:06Z

pyphare/pyphare/pharein/simulation.py

        grid = GridLayout()
-        return max(grid.nbrGhosts(kwargs["interp_order"], x) for x in ['primal','dual'])
+        return max(
+            grid.nbrGhosts(kwargs["interp_order"], x) for x in ["primal", "dual"]
+        )


The function get_max_ghosts is defined inside another function, which can make the code harder to test and debug. Consider moving this function to the module level.

coderabbitai · 2023-11-09T17:49:06Z

pyphare/pyphare/pharein/simulation.py

+                raise ValueError(
+                    f"invalid option ({key}), valid options are {valid_keys}"
+                )

        valid_modes = ["conserve", "overwrite"]
        if "mode" not in restart_options:
-            raise ValueError (f"Restart mode not set, valid modes are {valid_modes}")
+            raise ValueError(f"Restart mode not set, valid modes are {valid_modes}")

        if "dir" in restart_options:
            restart_options["dir"] = check_directory(restart_options["dir"], "restarts")

        mode = restart_options["mode"]
        if mode not in valid_modes:
-            raise ValueError (f"Invalid restart mode {mode}, valid modes are {valid_modes}")
+            raise ValueError(
+                f"Invalid restart mode {mode}, valid modes are {valid_modes}"
+            )


The code checks if mode is in restart_options and raises an error if it's not. However, it doesn't handle the case where mode is not in valid_modes. Consider adding a check for this case.

coderabbitai · 2023-11-09T17:49:06Z

pyphare/pyphare/pharein/simulation.py

+        kwargs["strict"] = kwargs.get("strict", False)

        kwargs["dl"] = dl
-        kwargs["cells"] =  cells
+        kwargs["cells"] = cells


The code assigns kwargs["dl"] and kwargs["cells"] after calling check_domain(**kwargs). However, it doesn't handle the case where check_domain(**kwargs) raises an exception. Consider adding error handling for this case.

coderabbitai · 2023-11-09T17:49:06Z

pyphare/pyphare/pharein/simulation.py

+        kwargs["diag_export_format"] = kwargs.get("diag_export_format", "hdf5")
+        assert kwargs["diag_export_format"] in ["hdf5"]  # only hdf5 supported for now



The code asserts that diag_export_format is in ["hdf5"]. However, it doesn't handle the case where diag_export_format is not in kwargs. Consider using the get method to fetch the value of diag_export_format with a default value.

- kwargs["diag_export_format"] = kwargs.get("diag_export_format", "hdf5") + kwargs["diag_export_format"] = kwargs.get("diag_export_format", default_value)

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.

Suggested change

kwargs["diag_export_format"] = kwargs.get("diag_export_format", "hdf5")

assert kwargs["diag_export_format"] in ["hdf5"] # only hdf5 supported for now

kwargs["diag_export_format"] = kwargs.get("diag_export_format", default_value)

assert kwargs["diag_export_format"] in ["hdf5"] # only hdf5 supported for now

coderabbitai · 2023-11-09T17:49:06Z

pyphare/pyphare/pharein/simulation.py

        kwargs["refinement"] = check_refinement(**kwargs)
        if kwargs["refinement"] == "boxes":
-            kwargs["refinement_boxes"], kwargs["max_nbr_levels"] = check_refinement_boxes(ndim, **kwargs)
+            (
+                kwargs["refinement_boxes"],
+                kwargs["max_nbr_levels"],
+            ) = check_refinement_boxes(ndim, **kwargs)
        else:
-            kwargs["max_nbr_levels"] = kwargs.get('max_nbr_levels', None)
-            assert kwargs["max_nbr_levels"] != None # this needs setting otherwise
+            kwargs["max_nbr_levels"] = kwargs.get("max_nbr_levels", None)
+            assert kwargs["max_nbr_levels"] != None  # this needs setting otherwise


The code checks if refinement is equal to "boxes" and assigns kwargs["refinement_boxes"] and kwargs["max_nbr_levels"] accordingly. However, it doesn't handle the case where check_refinement_boxes(ndim, **kwargs) raises an exception. Consider adding error handling for this case.

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between da875f4 and 7ff66f3.

Files selected for processing (2)

src/diagnostic/detail/types/fluid.hpp (10 hunks)
tests/diagnostic/test_diagnostics.ipp (1 hunks)

Additional comments: 12

tests/diagnostic/test_diagnostics.ipp (1)

16-27: The changes correctly add the new diagnostics for the momentum tensor of ions and protons. This is consistent with the pull request summary, which mentions the introduction of tensor fields and updates to the diagnostics to handle these new fields. The chaining of addDiagDict calls is a clean way to add multiple diagnostics in a concise manner. However, ensure that the Hi5Diagnostic class and its addDiagDict method are designed to handle the new tensor field diagnostics without any issues.

src/diagnostic/detail/types/fluid.hpp (11)

2-8: The inclusion of the new header file core/numerics/interpolator/interpolator.hpp is necessary for the FluidDiagnosticWriter class to use the interpolator functionality. This change is appropriate given the context of the pull request, which involves updating the diagnostic writer to handle tensor fields and momentum tensor computation.

34-38: The use of static constexpr for dimension and interp_order is a good practice as it allows the compiler to optimize the code where these constants are used. However, ensure that the GridLayout class indeed has these members and that they are also static constexpr.

46-47: The addition of the compute member function is a significant change. It is important to ensure that this function is called at the appropriate times in the simulation lifecycle to compute the momentum tensor when needed.

58-60: The override of writeAttributes without providing an implementation in this hunk could lead to linker errors if the function is not defined elsewhere. Ensure that the implementation is provided in the corresponding source file.

62-67: The isActiveDiag function is a private utility function that checks if a diagnostic is active based on its name. It is a good encapsulation of a repeated check, which improves code readability and maintainability.

73-138: The compute function is responsible for computing the momentum tensor for each ion population. It uses a lambda function to perform the computation for each population and for the ions as a whole. This is a complex function, and it is important to ensure that it is thoroughly tested to avoid any issues with the momentum tensor computation.

144-154: The createFiles function is updated to handle the creation of files for the new momentum tensor diagnostic. It is important to ensure that the file creation process is tested to handle the new tensor field data correctly.

183-221: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [165-213]

The getDataSetInfo function is updated to handle tensor fields. It uses lambda functions to set the dataset information for scalar, vector, and tensor fields. This is a good use of lambda functions to avoid code duplication and improve maintainability.

263-283: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [220-283]

The initDataSets function is updated to initialize datasets for the new tensor fields. It uses lambda functions to initialize scalar, vector, and tensor fields, which is a good practice to avoid code duplication. Ensure that the initialization process is tested to handle the new tensor field data correctly.

291-320: The write function is updated to write the computed tensor fields to the HDF5 files. It uses lambda functions to write scalar, vector, and tensor fields. This is a good practice to avoid code duplication and improve maintainability. Ensure that the writing process is tested to handle the new tensor field data correctly.

340-346: The writeAttributes function is updated to write attributes for the new momentum tensor diagnostic. It is important to ensure that the attribute writing process is tested to handle the new tensor field data correctly.

src/core/numerics/interpolator/interpolator.hpp

-        auto const& [xStartIndex]         = startIndex;
-        auto const& [xWeights]            = weights;
-        auto const& order_size            = xWeights.size();
+        auto const& [xStartIndex] = startIndex;


src/core/data/tensorfield/tensorfield.hpp

+    template<std::size_t... Index>
+    NO_DISCARD auto components(std::index_sequence<sizeof...(Index)>)
+    {
+        return std::forward_as_tuple((*this)[Index]...); //(*this)[0], (*this)[1], ...);


src/core/data/tensorfield/tensorfield.hpp

+    template<std::size_t... Index>
+    NO_DISCARD auto components(std::index_sequence<Index...>) const
+    {
+        return std::forward_as_tuple((*this)[Index]...); //(*this)[0], (*this)[1], ...);


src/core/numerics/interpolator/interpolator.hpp

+        auto end                             = particleRange.end();
+        auto& startIndex_                    = this->primal_startIndex_;
+        auto& weights_                       = this->primal_weights_;
+        auto const& [xx, xy, xz, yy, yz, zz] = momentumTensor();


src/core/numerics/interpolator/interpolator.hpp

+        auto end                          = particleRange.end();
+        auto& startIndex_                 = primal_startIndex_;
+        auto& weights_                    = primal_weights_;
+        auto const& [xFlux, yFlux, zFlux] = flux();


src/core/numerics/interpolator/interpolator.hpp

-        auto const& [xWeights]            = weights;
-        auto const& order_size            = xWeights.size();
+        auto const& [xStartIndex] = startIndex;
+        auto const& [xWeights]    = weights;


src/core/data/tensorfield/tensorfield.hpp

+    void setBuffer(std::string const& bufferName, field_type* field)
+    {
+        if (auto it = nameToIndex_.find(bufferName); it != std::end(nameToIndex_))
+            components_[it->second] = field;


coderabbitai

Review Status

Actionable comments generated: 15

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 2ae567b and 68a2297.

Files selected for processing (39)

pyphare/pyphare/core/gridlayout.py (3 hunks)
pyphare/pyphare/pharein/init.py (10 hunks)
pyphare/pyphare/pharein/diagnostics.py (5 hunks)
pyphare/pyphare/pharein/simulation.py (18 hunks)
pyphare/pyphare/pharesee/hierarchy.py (5 hunks)
pyphare/pyphare/pharesee/run.py (7 hunks)
src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 hunks)
src/amr/physical_models/hybrid_model.hpp (1 hunks)
src/core/CMakeLists.txt (1 hunks)
src/core/data/grid/gridlayoutdefs.hpp (1 hunks)
src/core/data/grid/gridlayoutimplyee.hpp (4 hunks)
src/core/data/ions/ion_population/ion_population.hpp (8 hunks)
src/core/data/ions/ions.hpp (10 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield.hpp (2 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/core/hybrid/hybrid_quantities.hpp (2 hunks)
src/core/numerics/interpolator/interpolator.hpp (11 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
src/diagnostic/detail/types/particle.hpp (1 hunks)
src/diagnostic/diagnostics.hpp (1 hunks)
src/phare_core.hpp (3 hunks)
src/python3/patch_level.hpp (6 hunks)
src/restarts/detail/h5writer.hpp (1 hunks)
tests/amr/models/test_models.cpp (3 hunks)
tests/amr/resources_manager/test_resources_manager.cpp (3 hunks)
tests/core/data/electrons/test_electrons.cpp (6 hunks)
tests/core/data/ion_population/test_ion_population.cpp (3 hunks)
tests/core/data/ions/test_ions.cpp (2 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
tests/diagnostic/init.py (3 hunks)
tests/diagnostic/test_diagnostics.hpp (2 hunks)
tests/diagnostic/test_diagnostics.ipp (1 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (2 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Files skipped from review due to trivial changes (2)

pyphare/pyphare/pharein/simulation.py
pyphare/pyphare/pharesee/run.py

Additional comments: 108

pyphare/pyphare/core/gridlayout.py (3)

31-40: The addition of new momentum tensor components "Mxx", "Mxy", "Mxz", "Myy", "Myz", "Mzz" with "primal" centering under the "x" section of the yee_centering dictionary is consistent with the PR's objective to generalize the ParticleToMesh operator.

60-69: The addition of new momentum tensor components "Mxx", "Mxy", "Mxz", "Myy", "Myz", "Mzz" with "primal" centering under the "y" section of the yee_centering dictionary is consistent with the PR's objective to generalize the ParticleToMesh operator.

89-98: The addition of new momentum tensor components "Mxx", "Mxy", "Mxz", "Myy", "Myz", "Mzz" with "primal" centering under the "z" section of the yee_centering dictionary is consistent with the PR's objective to generalize the ParticleToMesh operator.

pyphare/pyphare/pharein/__init__.py (16)

10-11: The global variable PHARE_EXE is set to False and never changed. If it's not used elsewhere, consider removing it.

13-29: The environment variable VIRTUAL_ENV is used to construct a path to the Python executable and modify sys.path. Ensure that this logic is necessary and secure, as it can affect the execution environment of the script.

54-63: This function _patch_data_ids opens an HDF5 file but never closes it. This can lead to resource leaks. Consider using a context manager to ensure the file is properly closed.

66-71: Same issue as above with _serialized_simulation_string. The HDF5 file is opened but never closed.

76-82: The py_fn_wrapper class is introduced for wrapping Python functions. Ensure that this class is used consistently and that the wrapped functions are properly handled in the context of the simulation.

88-103: The fn_wrapper class extends py_fn_wrapper to convert numpy arrays to C++ SubSpan. Verify that the conversion is correct and that the C++ side can handle the SubSpan objects without issues.

106-112: The clearDict function stops the dictator, which may be a global state manager. Ensure that this function is called at appropriate times to prevent unintended side effects.

120-131: The add_size_t function raises a RuntimeError if a negative value is received. Ensure that all calls to this function pass non-negative values.

180-187: The as_paths function is used to add refinement box paths to the configuration. Ensure that the refinement boxes are correctly formatted and that the paths are added correctly.

190-202: The logic for adding refinement box coordinates seems to assume a maximum of three dimensions. Verify that this assumption is valid for all use cases.

204-211: The conditional logic for setting the refinement tagging method should be verified to ensure that it aligns with the intended behavior for different refinement strategies.

224-245: The loop for adding ion populations and their properties should be verified to ensure that all necessary properties are set and that the population index is correctly handled.

306-330: The restart logic includes deserialization of simulation parameters and validation of compatibility. Ensure that the deserialization and validation logic is robust and handles all edge cases.

335-346: The logic for adding restart options should be verified to ensure that all options are correctly handled and that the paths are set appropriately.

352-359: The logic for adding electron properties should be verified to ensure that all properties are correctly handled and that the paths are set appropriately.

1-3: The summary mentions changes to function and class declarations, but the provided hunks do not show any changes to these declarations. This might indicate an inconsistency between the summary and the provided hunks.

pyphare/pyphare/pharein/diagnostics.py (3)

10-10: The change from single to double quotes for string literals is a style preference and is consistent throughout the changes.

Also applies to: 22-27

57-74: The changes to exception messages in the validate_timestamps function improve readability and are consistent with modern Python string formatting practices.

267-282: The refactoring of the FluidDiagnostics class, including the new function for_total_ions and the different handling of "pressure_tensor", aligns with the PR objective of generalizing the ParticleToMesh operator.

pyphare/pyphare/pharesee/hierarchy.py (4)

147-151: The addition of a detailed error message in the FieldData class __init__ method improves clarity and maintainability.

171-181: Refactoring the meshgrid method to include the grid inner function and conditional logic based on self.ndim enhances readability and modularity.

1162-1173: The addition of new quantities such as momentum tensor components to the field_qties dictionary aligns with the PR objectives to generalize the ParticleToMesh operator.

1537-1546: The isFieldQty function has been updated with additional strings to check for new field quantities, which is consistent with the changes in the field_qties dictionary.
src/amr/data/field/coarsening/field_coarsen_operator.hpp (3)
57-60: The return type of getOperatorPriority is correctly set to int, which aligns with the expected behavior of the method.

74-77: The NO_DISCARD attribute has been removed from getStencilWidth. Ensure that the return value of this method is always used or checked to prevent potential bugs from ignored return values.

74-77: The summary states that a stencil width of 5 is sufficient, but the code sets it to 2. Please verify that the stencil width is set correctly according to the requirements.
-            return SAMRAI::hier::IntVector{dim, 2};
+            return SAMRAI::hier::IntVector{dim, 5};
src/amr/physical_models/hybrid_model.hpp (4)

38-38: The summary indicates that the resources_manager_type declaration was removed, but it is still present in the code. Please verify this discrepancy.

37-42: The summary does not mention the addition of particle_array_type and ParticleInitializerFactory. These are significant changes that should be included in the summary.

29-30: The summary does not mention the addition of patch_t and level_t type aliases. These are significant changes that should be included in the summary.

34-36: The summary does not mention the addition of vecfield_type, field_type, and ions_type. These are significant changes that should be included in the summary.

src/core/CMakeLists.txt (1)

23-23: The addition of tensorfield.hpp to the SOURCES_INC list is noted and aligns with the PR's objective to introduce tensor field functionality.

src/core/data/grid/gridlayoutdefs.hpp (1)

74-90: The addition of new constants iMxx, iMxy, iMxz, iMyy, iMyz, and iMzz to the gridDataT struct aligns with the PR's objective to generalize the ParticleToMesh operator for various quantities computation. These constants are correctly assigned corresponding scalar values from HybridQuantity::Scalar.

src/core/data/grid/gridlayoutimplyee.hpp (4)

91-112: The addition of new QtyCentering arrays for momentum tensor components (Mxx, Mxy, Mxz, Myy, Myz, Mzz) and their inclusion in the hybridQtyCentering array aligns with the PR objectives to generalize the ParticleToMesh operator for various quantities computation.

164-179: The switch statement in the centering function for 1D case has been correctly updated to include the new momentum tensor components (Mxx, Mxy, Mxz, Myy, Myz, Mzz).

226-247: The switch statement in the centering function for 2D case has been correctly updated to include the new momentum tensor components (Mxx, Mxy, Mxz, Myy, Myz, Mzz).

308-337: The switch statement in the centering function for 3D case has been correctly updated to include the new momentum tensor components (Mxx, Mxy, Mxz, Myy, Myz, Mzz).

src/core/data/ions/ion_population/ion_population.hpp (8)

21-24: The addition of the TensorField template parameter to the IonPopulation class is consistent with the PR objectives to generalize the ParticleToMesh operator.

27-40: The new member momentumTensor_ has been added to the IonPopulation class, aligning with the PR's goal to compute various quantities from particle data.

51-63: The isUsable and isSettable methods have been updated to include checks for the momentumTensor_ member, ensuring that the new tensor field is integrated into the class's state checks.

191-195: The getter methods for the flux_ and momentumTensor_ members are correctly implemented, providing both const and non-const access.

208-214: The MomentProperties type has been changed from std::vector to std::array, which is a more appropriate choice for fixed-size collections and can potentially offer better performance.

227-231: The ParticleProperties type has been changed from std::vector to std::array, which is consistent with the changes made to MomentProperties and aligns with the PR's objectives.

261-264: The getCompileTimeResourcesUserList method has been updated to include the momentumTensor_ in its return value, ensuring that the new tensor field is considered when the class interacts with other parts of the codebase.

283-289: The private member variables have been updated to include the momentumTensor_ and the rho_ pointer has been initialized to nullptr, which is a good practice to avoid uninitialized pointers.

src/core/data/ions/ions.hpp (2)

194-202: The isUsable() function now checks the usability of the momentumTensor_ in addition to other members. Ensure that all conditions are correctly implemented and that the momentumTensor_ is indeed required for the Ions class to be considered usable.

207-215: Similarly, the isSettable() function checks the settable state of the momentumTensor_. Verify that the logic for determining if the Ions class is settable is correct and consistent with the intended use cases.

src/core/data/vecfield/vecfield.hpp (1)

17-18: The change from a class to a using declaration for VecField aligns with the PR objectives to generalize the ParticleToMesh operator and is consistent with the summary provided.

src/core/data/vecfield/vecfield_component.hpp (5)

12-12: The addition of new enum class Component values for tensor components aligns with the PR's objective to generalize the ParticleToMesh operator.

16-24: The componentMap function template correctly handles different ranks and provides a clear error message for unsupported ranks.

26-29: The at function's use of componentMap and std::forward is a good practice for encapsulation and argument forwarding.

35-40: The check function has been correctly updated to include the new tensor components.

43-47: The inline definitions of vecComponentMap and tensComponentMap are correctly implemented and provide the necessary mappings for vector and tensor components.

src/core/hybrid/hybrid_quantities.hpp (1)

13-40: The addition of new components to the Scalar enum class and the introduction of the Vector and Tensor enum classes, as well as the TensorType template, are consistent with the PR objectives to generalize the ParticleToMesh operator.

src/core/numerics/interpolator/interpolator.hpp (7)

281-299: The ParticleToMesh<1> specialization has been updated to use a Func parameter, which allows for the generalization of the deposit calculation function. This aligns with the PR's objective to compute various quantities from particle data.

306-321: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [306-330]

The ParticleToMesh<2> specialization has also been updated similarly to the ParticleToMesh<1> specialization, using a Func parameter for the deposit calculation function. This change supports the PR's objective.

337-352: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [337-364]

The ParticleToMesh<3> specialization reflects the same pattern of changes as the 1D and 2D specializations, using a Func parameter for the deposit calculation function. This consistency across dimensions is good for maintainability and understanding of the code.

374-377: The Interpolator class now has a protected section, which suggests that there may be new derived classes or a change in the way the class is intended to be used. This should be consistent with the introduction of the MomentumTensorInterpolator class.

477-508: The Interpolator class's operator() method for particle range processing has been updated to use the ParticleToMesh class for depositing density and flux. This change should be verified to ensure that it integrates correctly with the rest of the codebase and that the Func parameter is being used appropriately.

574-615: The new MomentumTensorInterpolator class inherits from Interpolator and provides functionality to compute the momentum tensor. This addition should be verified to ensure it aligns with the PR's objective to compute various quantities from particle data.

6-15: The file inclusions have been updated to reflect the new requirements of the code, such as the inclusion of hybrid_quantities and gridlayoutdefs. This change should be verified to ensure that all necessary headers are included and that there are no missing dependencies.

src/diagnostic/detail/h5writer.hpp (3)

37-48: The class Writer has been correctly renamed to H5Writer, and the template parameter H5Writer has been renamed to Writer. This aligns with the PR objectives and the summary provided.

37-48: The member functions and data members of the H5Writer class have been updated to reflect the renaming of the class. The use of This as an alias for H5Writer<ModelView> is consistent throughout the file.

142-149: The method writeVecFieldAsDataset has been correctly renamed to writeTensorFieldAsDataset and updated to handle tensor fields, as indicated by the existing comment.

src/diagnostic/detail/types/electromag.hpp (1)

152-156: The change from writeVecFieldAsDataset to writeTensorFieldAsDataset aligns with the generalization of the codebase to handle tensor fields.

src/diagnostic/detail/types/fluid.hpp (9)

2-8: The addition of the include statement for "core/numerics/interpolator/interpolator.hpp" is consistent with the changes described in the summary.

34-48: The addition of new static constexpr member variables dimension and interp_order and the new private method isActiveDiag in the FluidDiagnosticWriter class are consistent with the changes described in the summary.

58-67: The changes in the getDataSetInfo, initDataSets, write, and writeAttributes methods to handle "momentum_tensor" data are consistent with the changes described in the summary.

74-139: The re-implementation of the compute method to use a MomentumTensorInterpolator and perform computations based on the diagnostic activity is consistent with the changes described in the summary.

144-154: The changes in the createFiles method to create files for "momentum_tensor" in addition to "density" and "bulkVelocity" are consistent with the changes described in the summary.

183-221: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [160-214]

The changes in the getDataSetInfo method to include handling of "momentum_tensor" data are consistent with the changes described in the summary.

263-283: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [220-283]

The changes in the initDataSets method to include handling of "momentum_tensor" data are consistent with the changes described in the summary.

291-320: The changes in the write method to include handling of "momentum_tensor" data are consistent with the changes described in the summary.

340-346: The changes in the writeAttributes method to include handling of "momentum_tensor" data are consistent with the changes described in the summary.

src/diagnostic/detail/types/particle.hpp (3)

6-7: The import for "core/data/grid/gridlayout.hpp" has been added as indicated in the summary. This aligns with the PR objective to generalize the ParticleToMesh operator and suggests a shift in the code's dependency on grid-related data structures.

4-10: The summary mentions the removal of "amr/data/particles/particles_data.hpp", but this change is not visible in the provided hunk. The hunk only shows the final state of the code, so the removal cannot be confirmed here.

4-10: The structure and implementation of the ParticlesDiagnosticWriter template class appear to be consistent with the PR objectives of generalizing the ParticleToMesh operator to compute various quantities from particle data.

src/diagnostic/diagnostics.hpp (1)

55-59: The change from h5::Writer<ModelView_t> to h5::H5Writer<ModelView_t> for Writer_t is a significant alteration to an exported entity. Ensure that all dependent code has been updated to accommodate this change, as it may affect the logic related to data writing and storage.

src/phare_core.hpp (2)

1-3: The header guard is correctly implemented.

33-45: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [33-58]

The changes to the type aliases, including the introduction of SymTensorField_t and its integration into IonPopulation_t, Ions_t, and Electrons_t, are consistent with the PR objectives and the AI-generated summaries.

src/python3/patch_level.hpp (1)

82-82: The change from core::Components::componentMap to core::Components::componentMap() is consistent with the PR's objective to generalize the ParticleToMesh operator and the summary provided. This change likely reflects a design decision to encapsulate the component map within a method, which could be for reasons such as lazy initialization or providing a read-only view of the component map.

src/restarts/detail/h5writer.hpp (3)

1-9: The summary mentions the removal of "core/logger.hpp" and "core/utilities/types.hpp", but these removals are not reflected in the provided hunk. Please verify this discrepancy.

5-7: The inclusion of "core/utilities/mpi_utils.hpp" and "initializer/data_provider.hpp" aligns with the summary.

6-6: The inclusion of "restarts/restarts_props.hpp" is still present in the hunk, which contradicts the existing comment about its removal.

tests/amr/models/test_models.cpp (3)

43-43: The summary indicates that VecField1D is no longer used in the declaration of Electrons1D, but it is still being used in the declaration of Electromag1D. Please verify if this is intentional or an oversight.

45-45: The summary indicates that Electrons1D is no longer used in the declaration of Ions1D, but it is still being used in the declaration of HybridModelT. Please verify if this is intentional or an oversight.

41-41: The addition of SymTensorField1D to the IonsPop1D declaration aligns with the PR objectives to handle tensor fields for various quantities computation.

tests/amr/resources_manager/test_resources_manager.cpp (1)

1-12: The summary mentions the removal of an import statement for "core/data/grid/gridlayout_impl.hpp", but this change is not reflected in the provided hunks. Please verify if this import was indeed removed and if the summary needs to be updated.

tests/core/data/electrons/test_electrons.cpp (6)

1-10: The inclusion of new headers for tensor fields and related classes is consistent with the changes introduced in the PR.

127-137: The declarations of new types and aliases such as VecFieldND, SymTensorFieldND, IonPopulationND, IonsT, PartPackND, and StandardHybridElectronFluxComputerT are consistent with the PR objectives to handle new tensor quantities.

149-164: The introduction of new fields for momentum tensors and fluxes aligns with the PR objectives to compute various quantities from particle data.

188-214: The allocation of memory for the new fields related to momentum tensors and fluxes is necessary for the new functionality to compute pressure and other quantities.

232-243: Setting buffers for the momentum tensor in the Ions object is consistent with the changes to handle momentum tensors.

247-257: Setting buffers for the momentum tensor in the IonPopulation objects is consistent with the changes to handle momentum tensors.

tests/core/data/ion_population/test_ion_population.cpp (2)

32-38: The addition of DummyTensorField with isUsable and isSettable methods aligns with the PR's objective to generalize the ParticleToMesh operator.

60-61: The modification of IonPopulation to include DummyTensorField as a template parameter is consistent with the PR's objective to handle tensor fields.

tests/core/data/ions/test_ions.cpp (1)

37-42: The summary states that VecField1D was replaced with SymTensorField1D, but the code shows that VecField1D is still being used alongside SymTensorField1D. Please clarify if this is intentional or if the summary needs to be updated.

tests/core/numerics/ion_updater/test_updater.cpp (3)

239-260: The addition of momentum tensor fields (Mxx, Mxy, Mxz, Myy, Myz, Mzz) and their variations for protons and alpha particles (protons_Mxx, protons_Mxy, etc.) is consistent with the PR objectives and the summary provided. These fields are crucial for the computation of various quantities from particle data, as described in the PR objectives.

298-337: The initialization of the new momentum tensor fields in the IonsBuffers constructor correctly uses the layout.allocSize method to allocate the appropriate size for each field. This ensures that the fields are ready for use in computations and is consistent with the initialization pattern for other fields in the class.

471-483: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [471-511]

The setBuffers method in the IonsBuffers class has been updated to include the new momentum tensor fields. This ensures that the Ions class has access to these fields, which is necessary for the computation of various quantities from particle data.

tests/diagnostic/__init__.py (2)

29-30: The addition of "pressure_tensor" to the diagnostic quantities is consistent with the PR objectives to compute various quantities from particle data.

38-38: The addition of "flux" to the diagnostic quantities is consistent with the PR objectives to compute various quantities from particle data.

tests/diagnostic/test_diagnostics.hpp (2)

39-39: The change from direct access to componentMap to calling componentMap() function should be verified for consistency across the codebase.

48-48: The update from Writer<ModelView_t> to H5Writer<ModelView_t> in the Hi5Diagnostic struct is correctly reflected in the summary.

tests/diagnostic/test_diagnostics.ipp (1)

16-26: The additions to the diagnostic dictionaries for momentum tensors align with the PR objectives and are correctly implemented.

tests/simulator/test_initialization.py (1)

713-719: The loop now iterates over key-value pairs of ph.global_vars.sim.diagnostics, which implies that the structure of diagnostics has changed to a dictionary. Ensure that all usages of ph.global_vars.sim.diagnostics throughout the codebase are updated to accommodate this change.

coderabbitai · 2023-12-06T10:36:12Z

src/core/hybrid/hybrid_quantities.hpp


        throw std::runtime_error("Error - invalid Vector");
    }
+    static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty)
+    {
+        // no condition, for now there's only then momentum tensor M
+        return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}};
+    }


Consider adding a condition to handle future tensor quantities in the componentsQuantities function for Tensor types to ensure the code is extensible for additional tensor quantities that may be added in the future.

static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty) { + if (qty == Tensor::M) return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}}; + else + throw std::runtime_error("Error - invalid Tensor"); }

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.

Suggested change

throw std::runtime_error("Error - invalid Vector");

}

static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty)

{

// no condition, for now there's only then momentum tensor M

return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}};

}

throw std::runtime_error("Error - invalid Vector");

}

static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty)

{

if (qty == Tensor::M)

return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}};

else

throw std::runtime_error("Error - invalid Tensor");

}

coderabbitai · 2023-12-06T10:36:12Z

src/diagnostic/detail/types/electromag.hpp

@@ -69,7 +69,7 @@ void ElectromagDiagnosticWriter<H5Writer>::getDataSetInfo(DiagnosticProperties&
    std::string lvlPatchID = std::to_string(iLevel) + "_" + patchID;

    auto infoVF = [&](auto& vecF, std::string name, auto& attr) {


The lambda function infoVF captures all variables from the outer scope by reference. Consider capturing only the necessary variables explicitly to improve readability and maintainability.

- auto infoVF = [&](auto& vecF, std::string name, auto& attr) { + auto infoVF = [&lvlPatchID, &patchAttributes](auto& vecF, std::string name, auto& attr) {

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.

Suggested change

auto infoVF = [&](auto& vecF, std::string name, auto& attr) {

auto infoVF = [&lvlPatchID, &patchAttributes](auto& vecF, std::string name, auto& attr) {

coderabbitai · 2023-12-06T10:36:12Z

src/diagnostic/detail/types/electromag.hpp

@@ -104,7 +104,7 @@
    auto vecFields = h5Writer.modelView().getElectromagFields();

    auto initVF = [&](auto& path, auto& attr, std::string key, auto null) {


The lambda function initVF captures all variables from the outer scope by reference. Consider capturing only the necessary variables explicitly to improve readability and maintainability.

- auto initVF = [&](auto& path, auto& attr, std::string key, auto null) { + auto initVF = [&h5file, &h5Writer, this](auto& path, auto& attr, std::string key, auto null) {

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.

Suggested change

auto initVF = [&](auto& path, auto& attr, std::string key, auto null) {

auto initVF = [&h5file, &h5Writer, this](auto& path, auto& attr, std::string key, auto null) {

😶‍🌫️

coderabbitai · 2023-12-06T10:36:13Z

src/diagnostic/detail/h5writer.hpp

    static constexpr std::size_t flush_never = 0;

    template<typename Hierarchy, typename Model>
-    Writer(Hierarchy& hier, Model& model, std::string const hifivePath,
-           unsigned _flags /* = HiFile::ReadWrite | HiFile::Create | HiFile::Truncate */)
+    H5Writer(Hierarchy& hier, Model& model, std::string const hifivePath,
+             unsigned _flags /* = HiFile::ReadWrite | HiFile::Create | HiFile::Truncate */)
        : flags{_flags}
        , filePath_{hifivePath}
        , modelView_{hier, model}
    {
    }


The constructor H5Writer does not initialize all member variables directly, which could potentially lead to uninitialized member usage. It's best practice to initialize all members to ensure they have valid values.

coderabbitai · 2023-12-06T10:36:13Z

src/diagnostic/detail/h5writer.hpp


    std::unordered_map<std::string, unsigned> file_flags;

-    std::unordered_map<std::string, std::shared_ptr<H5TypeWriter<This>>> writers{
+    std::unordered_map<std::string, std::shared_ptr<H5TypeWriter<This>>> typeWriters_{
        {"info", make_writer<MetaDiagnosticWriter<This>>()},
        {"fluid", make_writer<FluidDiagnosticWriter<This>>()},
        {"electromag", make_writer<ElectromagDiagnosticWriter<This>>()},


The typeWriters_ unordered_map is initialized with string literals as keys. It's a good practice to define such literals as constants to avoid typos and improve maintainability.

coderabbitai · 2023-12-06T10:36:13Z

pyphare/pyphare/pharesee/hierarchy.py

@@ -1249,6 +1255,41 @@ def compute_hier_from(h, compute):
    return PatchHierarchy(patch_levels, h.domain_box, refinement_ratio, time=t)


The compute_hier_from function is currently limited to single-time hierarchies. Consider documenting this limitation or extending the function to handle multi-time hierarchies.

coderabbitai · 2023-12-06T10:36:13Z

pyphare/pyphare/pharesee/hierarchy.py

+def are_compatible_hierarchies(hierarchies):
+    pass
+
+
+def compute_hier_from_(compute, hierarchies):
+    # check that hierarchies are compatible
+    # and take the first one as a reference
+    ref_hier = hierarchies[0]
+
+    patch_levels = {}
+    for ilvl in range(ref_hier.levelNbr()):
+        new_patches = {}
+        for ip in range(len(ref_hier.patch_levels[ilvl].patches)):
+            patches = [h.patch_levels[ilvl].patches[ip] for h in hierarchies]
+            patch_datas = {
+                k: v for p in patches for k, v in list(p.patch_datas.items())
+            }
+            new_patch_datas = {}
+            layout = patches[0].layout  # asssume all layouts are the same
+            datas = compute(patch_datas)
+            for data in datas:
+                pd = FieldData(
+                    layout, data["name"], data["data"], centering=data["centering"]
+                )
+                new_patch_datas[data["name"]] = pd
+            if ilvl not in new_patches:
+                new_patches[ilvl] = []
+            new_patches[ilvl].append(Patch(new_patch_datas, patches[0].id))
+
+        patch_levels[ilvl] = PatchLevel(ilvl, new_patches[ilvl])
+
+    t = list(ref_hier.time_hier.keys())[0]
+    return PatchHierarchy(patch_levels, ref_hier.domain_box, refinement_ratio, time=t)
+


The compute_hier_from_ function lacks implementation details for are_compatible_hierarchies. Ensure that compatibility checks are properly implemented to prevent potential issues when processing multiple hierarchies.

coderabbitai · 2023-12-06T10:36:14Z

src/core/data/tensorfield/tensorfield.hpp

+    NO_DISCARD field_type& getComponent(Component component)
+    {
+        if (isUsable())
+        {
+            switch (component)
+            {
+                case Component::X: return *components_[0];
+                case Component::Y: return *components_[1];
+                case Component::Z: return *components_[2];
+
+                case Component::XX: return *components_[0];
+                case Component::XY: return *components_[1];
+                case Component::XZ: return *components_[2];
+                case Component::YY: return *components_[3];
+                case Component::YZ: return *components_[4];
+                case Component::ZZ: return *components_[5];


There is a logical error in the getComponent method. For a tensor field of rank 2, the indices for components XX, XY, XZ, YY, YZ, ZZ should be 0, 1, 2, 3, 4, 5 respectively, but the method incorrectly maps XX, XY, XZ to indices 0, 1, 2 and does not account for YY, YZ, ZZ.

- case Component::XX: return *components_[0]; - case Component::XY: return *components_[1]; - case Component::XZ: return *components_[2]; + case Component::XX: return *components_[0]; + case Component::XY: return *components_[1]; + case Component::XZ: return *components_[2]; + case Component::YY: return *components_[3]; + case Component::YZ: return *components_[4]; + case Component::ZZ: return *components_[5];

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.

Suggested change

NO_DISCARD field_type& getComponent(Component component)

{

if (isUsable())

{

switch (component)

{

case Component::X: return *components_[0];

case Component::Y: return *components_[1];

case Component::Z: return *components_[2];

case Component::XX: return *components_[0];

case Component::XY: return *components_[1];

case Component::XZ: return *components_[2];

case Component::YY: return *components_[3];

case Component::YZ: return *components_[4];

case Component::ZZ: return *components_[5];

NO_DISCARD field_type& getComponent(Component component)

{

if (isUsable())

{

switch (component)

{

case Component::X: return *components_[0];

case Component::Y: return *components_[1];

case Component::Z: return *components_[2];

case Component::XX: return *components_[0];

case Component::XY: return *components_[1];

case Component::XZ: return *components_[2];

case Component::YY: return *components_[3];

case Component::YZ: return *components_[4];

case Component::ZZ: return *components_[5];

coderabbitai · 2023-12-06T10:36:14Z

src/core/data/tensorfield/tensorfield.hpp

+    NO_DISCARD field_type const& getComponent(Component component) const
+    {
+        if (isUsable())
+        {
+            switch (component)
+            {
+                case Component::X: return *components_[0];
+                case Component::Y: return *components_[1];
+                case Component::Z: return *components_[2];
+
+                case Component::XX: return *components_[0];
+                case Component::XY: return *components_[1];
+                case Component::XZ: return *components_[2];
+                case Component::YY: return *components_[3];
+                case Component::YZ: return *components_[4];
+                case Component::ZZ: return *components_[5];


The same logical error is present in the getComponent const overload method. The indices for components XX, XY, XZ, YY, YZ, ZZ should be 0, 1, 2, 3, 4, 5 respectively, but the method incorrectly maps XX, XY, XZ to indices 0, 1, 2 and does not account for YY, YZ, ZZ.

- case Component::XX: return *components_[0]; - case Component::XY: return *components_[1]; - case Component::XZ: return *components_[2]; + case Component::XX: return *components_[0]; + case Component::XY: return *components_[1]; + case Component::XZ: return *components_[2]; + case Component::YY: return *components_[3]; + case Component::YZ: return *components_[4]; + case Component::ZZ: return *components_[5];

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.

Suggested change

NO_DISCARD field_type const& getComponent(Component component) const

{

if (isUsable())

{

switch (component)

{

case Component::X: return *components_[0];

case Component::Y: return *components_[1];

case Component::Z: return *components_[2];

case Component::XX: return *components_[0];

case Component::XY: return *components_[1];

case Component::XZ: return *components_[2];

case Component::YY: return *components_[3];

case Component::YZ: return *components_[4];

case Component::ZZ: return *components_[5];

NO_DISCARD field_type const& getComponent(Component component) const

{

if (isUsable())

{

switch (component)

{

case Component::X: return *components_[0];

case Component::Y: return *components_[1];

case Component::Z: return *components_[2];

case Component::XX: return *components_[0];

case Component::XY: return *components_[1];

case Component::XZ: return *components_[2];

case Component::YY: return *components_[3];

case Component::YZ: return *components_[4];

case Component::ZZ: return *components_[5];

coderabbitai · 2023-12-06T10:36:14Z

src/core/data/tensorfield/tensorfield.hpp

+    NO_DISCARD std::string getComponentName(Component component) const
+    {
+        switch (component)
+        {
+            case Component::X: return componentNames_[0];
+            case Component::Y: return componentNames_[1];
+            case Component::Z: return componentNames_[2];
+            case Component::XX: return componentNames_[0];
+            case Component::XY: return componentNames_[1];
+            case Component::XZ: return componentNames_[2];
+            case Component::YY: return componentNames_[3];
+            case Component::YZ: return componentNames_[4];
+            case Component::ZZ: return componentNames_[5];


The getComponentName method also has the same logical error as the getComponent methods. The indices for components XX, XY, XZ, YY, YZ, ZZ should be 0, 1, 2, 3, 4, 5 respectively, but the method incorrectly maps XX, XY, XZ to indices 0, 1, 2 and does not account for YY, YZ, ZZ.

- case Component::XX: return componentNames_[0]; - case Component::XY: return componentNames_[1]; - case Component::XZ: return componentNames_[2]; + case Component::XX: return componentNames_[0]; + case Component::XY: return componentNames_[1]; + case Component::XZ: return componentNames_[2]; + case Component::YY: return componentNames_[3]; + case Component::YZ: return componentNames_[4]; + case Component::ZZ: return componentNames_[5];

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.

Suggested change

NO_DISCARD std::string getComponentName(Component component) const

{

switch (component)

{

case Component::X: return componentNames_[0];

case Component::Y: return componentNames_[1];

case Component::Z: return componentNames_[2];

case Component::XX: return componentNames_[0];

case Component::XY: return componentNames_[1];

case Component::XZ: return componentNames_[2];

case Component::YY: return componentNames_[3];

case Component::YZ: return componentNames_[4];

case Component::ZZ: return componentNames_[5];

NO_DISCARD std::string getComponentName(Component component) const

{

switch (component)

{

case Component::X: return componentNames_[0];

case Component::Y: return componentNames_[1];

case Component::Z: return componentNames_[2];

case Component::XX: return componentNames_[0];

case Component::XY: return componentNames_[1];

case Component::XZ: return componentNames_[2];

case Component::YY: return componentNames_[3];

case Component::YZ: return componentNames_[4];

case Component::ZZ: return componentNames_[5];

PhilipDeegan · 2023-12-06T10:39:52Z

pyphare/pyphare/pharein/__init__.py

@@ -11,14 +10,20 @@
 #    which is called from Pybind when phare-exe (or similar) is in use
 PHARE_EXE = False

-venv_path = os.environ.get('VIRTUAL_ENV')
+venv_path = os.environ.get("VIRTUAL_ENV")

 if venv_path is not None:


@nicolasaunai do you use this anymore?

I generally don't run phare-exe, but that could be useful for any user no?

PhilipDeegan · 2023-12-06T10:42:05Z

pyphare/pyphare/pharein/__init__.py

@@ -1,4 +1,3 @@
-


maybe we can black these files on a standalone PR?

I'll try to do that yes you're right

coderabbitai

Review Status

Actionable comments generated: 4

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 68a2297 and dd74cf6.

Files selected for processing (4)

pyphare/pyphare/pharein/diagnostics.py (5 hunks)
pyphare/pyphare/pharesee/hierarchy.py (5 hunks)
src/core/data/ions/ions.hpp (12 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)

Additional comments: 19

pyphare/pyphare/pharein/diagnostics.py (1)

273-288: The implementation of the for_total_ions function and the handling of the "pressure_tensor" quantity in the FluidDiagnostics class align with the summary.

pyphare/pyphare/pharesee/hierarchy.py (4)

171-181: The addition of the grid inner function in the meshgrid method is a good refactor that simplifies the handling of different dimensions. This change improves code readability and maintainability.

1162-1174: The update to the field_qties dictionary to include momentum tensor components is consistent with the PR's objective to generalize the computation of various quantities based on particles.

1256-1256: The existing comment about compute_hier_from being limited to single-time hierarchies is still valid. It would be beneficial to document this limitation within the function's docstring or consider extending its capabilities to handle multi-time hierarchies.

1591-1600: The update to the isFieldQty function to include momentum tensor components is consistent with the PR's objective to generalize the computation of various quantities based on particles.

src/core/data/ions/ions.hpp (7)

7-21: The summary indicates that includes for ion_population/ion_population.hpp and ndarray/ndarray_vector.hpp were removed, but the hunks do not show any removed code. Please verify if these removals took place and update the summary accordingly.

10-11: The addition of #include <vector> and #include <array> is confirmed and aligns with the summary.

29-35: The introduction of tensorfield_type and its usage in member functions is confirmed and aligns with the summary.

47-51: The addition of the momentumTensor_ member variable is confirmed and aligns with the summary.

257-265: The summary indicates that the type of MomentProperties was changed from std::vector to std::array, but the hunks still show it as std::vector. Please verify this discrepancy.

185-200: The modifications to member functions to work with the new momentumTensor_ member variable are confirmed and align with the summary.

185-200: The addition of the computeFullMomentumTensor() function is confirmed and aligns with the summary.

src/diagnostic/detail/types/fluid.hpp (7)

2-7: The inclusion of the interpolator header and the vecfield_component.hpp is consistent with the changes made to support tensor field computations.

34-47: The addition of dimension and interp_order as static constexpr members and the changes to the compute method to include momentum tensor computation are aligned with the PR's goal to generalize particle-to-mesh operations.

144-154: The update to the createFiles method to handle the momentum tensor is consistent with the new tensor field functionality.

184-224: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [161-217]

The updates to getDataSetInfo to handle tensor fields and the momentum tensor are appropriate for the new diagnostic capabilities.

266-288: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [223-288]

The changes to initDataSets to initialize datasets for the momentum tensor align with the new functionality to handle tensor fields.

296-326: The updates to the write method to handle writing the momentum tensor to the datasets are consistent with the new tensor field functionality.

346-352: The update to writeAttributes to handle attributes for the momentum tensor is consistent with the new tensor field functionality.

coderabbitai · 2023-12-06T14:36:52Z

pyphare/pyphare/pharesee/hierarchy.py

                self.centerings = [kwargs["centering"]]
        else:
            raise ValueError(
-                "centering not specified and cannot be inferred from field name"
+                f"centering not specified and cannot be inferred from field name : {field_name}"
            )


The error message in the ValueError is informative, but consider adding more context about what valid field names might be or how to specify centering correctly to help users who encounter this error.

coderabbitai

Review Status

Actionable comments generated: 9

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 68a2297 and 1b25f15.

Files selected for processing (5)

pyphare/pyphare/pharein/diagnostics.py (5 hunks)
pyphare/pyphare/pharesee/hierarchy.py (5 hunks)
src/core/data/ions/ions.hpp (13 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)

Files skipped from review as they are similar to previous changes (1)

tests/core/numerics/ion_updater/test_updater.cpp

Additional comments: 14

pyphare/pyphare/pharesee/hierarchy.py (5)

147-151: The updated error message now includes the field name, which provides better context for the error. This change addresses the previous comment regarding the error message's clarity.

171-181: The refactoring of the meshgrid method with the addition of the inner grid function improves readability and encapsulates the logic for creating the mesh grid based on the number of dimensions.

1162-1174: The addition of new key-value pairs to the field_qties dictionary aligns with the PR objectives to support the computation of new quantities like the momentum tensor components.

1256-1256: The comment added to the compute_hier_from function now clearly states that it is currently limited to single-time hierarchies, addressing the previous suggestion to document this limitation.

1591-1600: The addition of new strings to the isFieldQty function is necessary for the function to recognize the new field quantities like the momentum tensor components.

src/core/data/ions/ions.hpp (1)

257-265: The summary indicates that MomentProperties has been changed from std::vector to std::array, but the code still uses std::vector. Please verify this discrepancy.

src/diagnostic/detail/types/fluid.hpp (8)

34-38: The addition of dimension and interp_order as static constexpr members provides clear, centralized definitions for these constants, which is good for maintainability.

62-67: The isActiveDiag method is a good encapsulation for checking if a diagnostic is active, improving readability and maintainability.

74-139: The compute method has been significantly expanded to handle the computation of the momentum tensor for ions. This change is consistent with the PR's objective to generalize the ParticleToMesh operator for various quantities.

144-154: The createFiles method now includes checks for creating files for the momentum tensor, which is a necessary change to support the new functionality introduced in the PR.

186-194: The infoVF and infoTF lambdas have been introduced to handle vector fields and tensor fields, respectively. This is a good use of lambdas to encapsulate functionality and improve code readability.

252-258: The initVF and initTF lambdas are used to initialize vector and tensor fields datasets. This is a good practice as it encapsulates the initialization logic and keeps the code DRY.

302-303: The writeTF lambda is introduced to write tensor fields to the dataset. This change is necessary to handle the new tensor field data introduced in the PR.

346-349: The writeAttributes method now includes a check for writing attributes related to the momentum tensor, which is consistent with the new functionality.

coderabbitai · 2023-12-06T15:37:23Z

pyphare/pyphare/pharein/diagnostics.py

    timestamps = phare_utilities.np_array_ify(kwargs.get(key, []))

    if np.any(timestamps < init_time):
-        raise RuntimeError(f"Error: timestamp({sim.time_step_nbr}) cannot be less than simulation.init_time({init_time}))")
+        raise RuntimeError(
+            f"Error: timestamp({sim.time_step_nbr}) cannot be less than simulation.init_time({init_time}))"
+        )
    if np.any(timestamps > sim.final_time):
-        raise RuntimeError(f"Error: timestamp({sim.time_step_nbr}) cannot be greater than simulation.final_time({sim.final_time}))")
+        raise RuntimeError(
+            f"Error: timestamp({sim.time_step_nbr}) cannot be greater than simulation.final_time({sim.final_time}))"
+        )
    if not np.all(np.diff(timestamps) >= 0):
        raise RuntimeError(f"Error: {clazz}.{key} not in ascending order)")
-    if not np.all(np.abs(timestamps / sim.time_step - np.rint(timestamps/sim.time_step) < 1e-9)):
-        raise RuntimeError(f"Error: {clazz}.{key} is inconsistent with simulation.time_step")
+    if not np.all(
+        np.abs(timestamps / sim.time_step - np.rint(timestamps / sim.time_step) < 1e-9)
+    ):
+        raise RuntimeError(
+            f"Error: {clazz}.{key} is inconsistent with simulation.time_step"
+        )


The error messages in the validate_timestamps function could be more informative by including the actual problematic timestamps. This would aid in debugging issues with timestamps that do not meet the validation criteria.

coderabbitai · 2023-12-06T15:37:24Z

pyphare/pyphare/pharein/diagnostics.py

@@ -81,203 +100,288 @@ def __init__(self, name, **kwargs):
            raise RuntimeError("A simulation must be created before adding diagnostics")


The error message in the Diagnostics class constructor could be more specific by stating that a Simulation object must be created before adding diagnostics.

coderabbitai · 2023-12-06T15:37:24Z

pyphare/pyphare/pharein/diagnostics.py

        if self.flush_every < 0:
-            raise RuntimeError(f"{self.__class__.__name__,}.flush_every cannot be negative")
-
-        if any([self.quantity == diagnostic.quantity for diagnostic in global_vars.sim.diagnostics]):
-            raise RuntimeError(f"Error: Diagnostic ({kwargs['quantity']}) already registered")
+            raise RuntimeError(
+                f"{self.__class__.__name__,}.flush_every cannot be negative"
+            )


The error message for flush_every in the Diagnostics class constructor could be more specific by stating that flush_every cannot be negative.

coderabbitai · 2023-12-06T15:37:24Z

pyphare/pyphare/pharein/diagnostics.py

+            error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(
+                ElectromagDiagnostics.em_quantities
+            )
+            raise ValueError(error_msg.format(kwargs["quantity"]))


The error message in the ElectromagDiagnostics class could be more specific by stating that the provided quantity is not a valid electromag diagnostics.

coderabbitai · 2023-12-06T15:37:24Z

pyphare/pyphare/pharein/diagnostics.py

+        if "extent" not in kwargs and self.quantity == "space_box":
+            raise ValueError(
+                "Error: missing 'extent' parameter required by 'space_box' the ParticleDiagnostics type"
+            )


The error message in the ParticleDiagnostics class could be more specific by stating that the 'extent' parameter is missing which is required by 'space_box' the ParticleDiagnostics type.

coderabbitai · 2023-12-06T15:37:24Z

pyphare/pyphare/pharein/diagnostics.py

+        if kwargs["quantity"] not in MetaDiagnostics.meta_quantities:
+            error_msg = "Error: '{}' not a valid meta diagnostics : " + ", ".join(
+                MetaDiagnostics.meta_quantities
+            )
+            raise ValueError(error_msg.format(kwargs["quantity"]))


The error message in the MetaDiagnostics class could be more specific by stating that the provided quantity is not a valid meta diagnostics.

coderabbitai · 2023-12-06T15:37:24Z

pyphare/pyphare/pharein/diagnostics.py

        self.population_name = None
-        if 'population_name' not in kwargs and kwargs['quantity'] == "flux":
+        if "population_name" not in kwargs and kwargs["quantity"] == "flux":
            raise ValueError("Error: missing population_name")
-        elif 'population_name' in kwargs:
-            self.population_name = kwargs['population_name']
-
-        if kwargs['quantity'] not in FluidDiagnostics.fluid_quantities:
-            error_msg = "Error: '{}' not a valid fluid diagnostics : " + ', '.join(FluidDiagnostics.fluid_quantities)
-            raise ValueError(error_msg.format(kwargs['quantity']))
-        elif kwargs['quantity'] == 'flux' and kwargs['population_name'] == "ions":
-            raise ValueError("'flux' is only available for specific populations, try 'bulkVelocity")
+        elif "population_name" in kwargs:
+            self.population_name = kwargs["population_name"]
+
+        if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
+            error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
+                FluidDiagnostics_.fluid_quantities
+            )
+            raise ValueError(error_msg.format(kwargs["quantity"]))
+        elif kwargs["quantity"] == "flux" and kwargs["population_name"] == "ions":
+            raise ValueError(
+                "'flux' is only available for specific populations, try 'bulkVelocity"
+            )
+        elif kwargs["quantity"] == "pressure_tensor":
+            raise ValueError("'pressure_tensor' is not available yet")


Consider making the error messages more specific by stating the exact issue with the provided quantity.

pyphare/pyphare/pharesee/run.py


    return jx, jy, jz


+def _compute_current(patchdatas, **kwargs):


pyphare/pyphare/pharesee/run.py


    else:
        raise RuntimeError("dimension not implemented")


-
-def _get_rank(patch):
+def _get_rank(patchdatas, **kwargs):


coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 1b25f15 and 5c54768.

Files selected for processing (2)

pyphare/pyphare/pharesee/hierarchy.py (7 hunks)
pyphare/pyphare/pharesee/run.py (7 hunks)

Files skipped from review due to trivial changes (1)

pyphare/pyphare/pharesee/run.py

Additional comments: 4

pyphare/pyphare/pharesee/hierarchy.py (4)

14-14: The PatchData class has been correctly updated to include a new meshgrid method and changes in the constructor to handle the field_name parameter.

1223-1223: The compute_hier_from function has been correctly renamed to compute_hier_from___ and a new compute_hier_from function has been added with different functionality.

1163-1175: Constants related to momentum tensors and density have been correctly added to the field_name_map dictionary.

1562-1571: The isFieldQty function has been correctly expanded to include additional field quantities related to momentum tensors.

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 5c54768 and 8ebeff3.

Files selected for processing (2)

pyphare/pyphare/pharesee/hierarchy.py (7 hunks)
pyphare/pyphare/pharesee/run.py (7 hunks)

Additional comments: 18

pyphare/pyphare/pharesee/hierarchy.py (2)

1318-1319: The change to the pop_name function modifies how the basename is split. Ensure that this change is consistent with the new file naming conventions and does not break existing functionality that relies on the previous behavior.

1562-1571: The modifications to the isFieldQty function to include new field quantities related to the momentum tensor are correct and align with the PR's objectives of adding pressure diagnostics.

pyphare/pyphare/pharesee/run.py (16)

14-26: The changes in _current1d function add explicit calculations for jy and jz and handle the boundary conditions by duplicating the edge values. This is a common approach for handling boundaries in grid-based computations. The comments indicate a hard-coded layout, suggesting that future work may involve generalizing the grid layout handling.

39-61: Similar to _current1d, the _current2d function has been updated to calculate the current components jx, jy, and jz and handle the boundary conditions. The code is consistent with the 1D case and follows the same pattern for handling boundaries.

66-103: The _compute_current function has been updated to handle both 1D and 2D cases for current computation. The function uses the dimensionality of the reference patch data to decide which current computation function to call. The return structure is consistent with the expected format for patch data.

106-109: The _divB2D function computes the divergence of the magnetic field in 2D. The calculations are straightforward and use central differences, which is a standard approach for numerical differentiation on a grid.

112-127: The _compute_divB function handles the computation of the divergence of the magnetic field. It raises a ValueError for 1D cases, which is appropriate since the divergence of a magnetic field in 1D is always zero by construction. The 2D case uses _divB2D for the actual computation.

159-185: The _compute_pressure function computes the pressure tensor components from the momentum tensor and mass density. The calculations are straightforward and follow the expected physical model for pressure computation.

188-216: The _compute_pop_pressure function is similar to _compute_pressure but for a specific population. It also takes into account the mass of the particles. The code is consistent with the physical model and the rest of the changes.

227-262: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [219-262]

The make_interpolator function has been updated to handle 1D and 2D cases for creating interpolators. The use of scipy.interpolate functions is appropriate for this task. The code is clean and follows best practices for conditional imports based on dimensionality.

269-284: The Run class constructor and _get_hierarchy method have been updated. The constructor now initializes self.hier to None, and _get_hierarchy uses a new helper function _get_hier to build the hierarchy from a file. The changes are consistent and improve code readability.

296-310: The _get method in the Run class has been updated to handle merged quantities. The changes include creating interpolators for each quantity in the hierarchy. The code is consistent with the rest of the changes and follows the PR's objectives.

313-378: New methods have been added to the Run class to retrieve various quantities like magnetic field, electric field, mass density, etc. These methods are consistent with the PR's objectives and provide a clear interface for accessing simulation data.

380-388: The GetParticles method in the Run class has been updated to handle a list of population names. The changes are straightforward and improve the method's flexibility.

390-404: The GetMass method in the Run class retrieves the mass of a particle population. The code ensures that all retrieved masses are the same, which is a good consistency check. However, the method assumes that the mass attribute is present in all related files, which might not always be the case.

Verify that the mass attribute is guaranteed to be present in all related files for a given population.

406-415: The GetDomainSize method in the Run class calculates the domain size based on the attributes of the magnetic field file. The calculation is straightforward, but the method has a TODO comment indicating that the file used could change in the future.

The method currently uses the magnetic field file to determine the domain size, but this may be subject to change.

425-451: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [417-445]

The GetDl method in the Run class calculates the grid sizes for a given level. The method handles the case where the level is specified as "finest" and calculates the grid size accordingly. The code is clear and follows the expected logic for grid size calculation.

447-451: The GetAllAvailableQties method in the Run class asserts that single_hier_for_all_quantities is True. This method retrieves all available quantities for a given time and set of populations. The assert statement ensures that the method is used correctly within the expected context.

coderabbitai · 2023-12-12T15:28:36Z

pyphare/pyphare/pharesee/hierarchy.py

+def compute_hier_from___(h, compute):
    """
    returns a hierarchy resulting from calling 'compute'
    on each patch of the given hierarchy 'h'


The function compute_hier_from___ appears to be a duplicate or incorrectly named, as there is a compute_hier_from function defined later in the code. Please verify if this is intentional and consider renaming or removing it if it's a duplicate.

nicolasaunai · 2023-12-14T14:04:48Z

pyphare/pyphare/pharesee/run.py

@@ -2,6 +2,7 @@

 import numpy as np
 from pyphare.pharesee.hierarchy import compute_hier_from
+from pyphare.pharesee.hierarchy import compute_hier_from_


pyphare/pyphare/pharesee/run.py

@@ -2,6 +2,7 @@

 import numpy as np
 from pyphare.pharesee.hierarchy import compute_hier_from
+from pyphare.pharesee.hierarchy import compute_hier_from_


pyphare/pyphare/pharein/__init__.py

PhilipDeegan · 2023-12-14T14:44:00Z

pyphare/pyphare/pharein/diagnostics.py

+                print(
+                    f"{diag.name} already registered {self.quantity}, merging timestamps"
+                )
+                my_times = self.write_timestamps


my -> new?

you could remove it altogether and use it (self.write_timestamps) at np.concatenate(

I think the proposed change is misleading. 'my' refers to 'self', 'new' refers to future self.
adding self.write_timestamps in the np.concatenates makes the line quite long and I find

' new_times = np.concatenate((my_times, existing_times))`

quite easy to read.

I don't think there is a perf issue as everything is just references anyway.

new IMO refers to additional, if there is existing

I have an aversion to my, implies personhood

PhilipDeegan · 2023-12-14T14:49:40Z

pyphare/pyphare/pharesee/run.py


        Jx, Jy, Jz = _current2d(Bx, By, Bz, dx, dy)
-        # return ({"name":"Jx", "data":Jx,"centering":"primal"},


PhilipDeegan · 2023-12-14T14:50:26Z

pyphare/pyphare/pharesee/run.py

-def _compute_divB(patch):
-    if patch.box.ndim == 1:
+def _compute_divB(patchdatas, **kwargs):
+


break here implies this file has not been formatted by black?

not sure what you mean I have black on save and this is version 22.8

doesn't look like it

diff --git a/pyphare/pyphare/pharesee/run.py b/pyphare/pyphare/pharesee/run.py index 0da598e..988479d 100644 --- a/pyphare/pyphare/pharesee/run.py +++ b/pyphare/pyphare/pharesee/run.py @@ -111,7 +111,6 @@ def _divB2D(Bx, By, xBx, yBy): def _compute_divB(patchdatas, **kwargs): - reference_pd = patchdatas["Bx"] # take Bx as a reference, but could be any other ndim = reference_pd.box.ndim

git status On branch pressure You are currently bisecting, started from branch 'master'. (use "git bisect reset" to get back to the original branch) Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) modified: pyphare/pyphare/pharein/diagnostics.py modified: pyphare/pyphare/pharesee/run.py

src/amr/data/field/coarsening/field_coarsen_operator.hpp

src/core/data/ions/ion_population/ion_population.hpp

src/core/data/ions/ions.hpp

PhilipDeegan · 2023-12-14T15:17:29Z

tests/simulator/test_diagnostics.py

-        model = setup_model()
-        dump_all_diags(model.populations)  # first register
-        self.assertRaises(RuntimeError, dump_all_diags, model.populations)
+    # def test_twice_register(self):


not sure why I did that...

PhilipDeegan · 2023-12-14T15:19:53Z

src/core/data/tensorfield/tensorfield.hpp

+
+
+
+    NO_DISCARD field_type& getComponent(Component component)


there's probably a way to generalize these

maybe something like https://gist.github.com/PhilipDeegan/b416b270260479dadd03218810d1c79c

tests/core/data/ion_population/test_ion_population.cpp

PhilipDeegan · 2023-12-14T15:22:16Z

src/restarts/detail/h5writer.hpp

 #include "restarts/restarts_props.hpp"
-
+#include "initializer/data_provider.hpp"


I don't get it

you don't get what?

data provider adds PhareDict

it was implicit before I guess, cause if it was needed it wouldn't compile without this,

just seems a bit odd on it's own without other edits in this file

src/diagnostic/detail/types/fluid.hpp

PhilipDeegan · 2023-12-17T18:25:56Z

src/core/data/grid/gridlayoutimplyee.hpp

@@ -91,11 +91,25 @@ namespace core
            const std::array<QtyCentering, NBR_COMPO> Vz
                = {{data.primal, data.primal, data.primal}};

+            const std::array<QtyCentering, NBR_COMPO> Mxx


clang-format-14 has an option to move consts to the right

QualifierAlignment: Right

I think

coderabbitai

Review Status

Actionable comments generated: 16

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 7948fab and 42cdad7.

Files selected for processing (40)

pyphare/pyphare/core/gridlayout.py (3 hunks)
pyphare/pyphare/pharein/init.py (1 hunks)
pyphare/pyphare/pharein/diagnostics.py (10 hunks)
pyphare/pyphare/pharein/simulation.py (4 hunks)
pyphare/pyphare/pharesee/hierarchy.py (7 hunks)
pyphare/pyphare/pharesee/run.py (6 hunks)
src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 hunks)
src/amr/physical_models/hybrid_model.hpp (1 hunks)
src/core/CMakeLists.txt (1 hunks)
src/core/data/grid/gridlayoutdefs.hpp (1 hunks)
src/core/data/grid/gridlayoutimplyee.hpp (4 hunks)
src/core/data/ions/ion_population/ion_population.hpp (8 hunks)
src/core/data/ions/ions.hpp (13 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield.hpp (2 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/core/hybrid/hybrid_quantities.hpp (2 hunks)
src/core/numerics/interpolator/interpolator.hpp (11 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
src/diagnostic/detail/types/particle.hpp (1 hunks)
src/diagnostic/diagnostics.hpp (1 hunks)
src/phare_core.hpp (3 hunks)
src/python3/patch_level.hpp (6 hunks)
src/restarts/detail/h5writer.hpp (1 hunks)
tests/amr/models/test_models.cpp (3 hunks)
tests/amr/resources_manager/test_resources_manager.cpp (3 hunks)
tests/core/data/electrons/test_electrons.cpp (6 hunks)
tests/core/data/gridlayout/test_laplacian.py (1 hunks)
tests/core/data/ion_population/test_ion_population.cpp (3 hunks)
tests/core/data/ions/test_ions.cpp (2 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
tests/diagnostic/init.py (2 hunks)
tests/diagnostic/test_diagnostics.hpp (2 hunks)
tests/diagnostic/test_diagnostics.ipp (1 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (2 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Files skipped from review due to trivial changes (3)

pyphare/pyphare/core/gridlayout.py
src/core/data/grid/gridlayoutdefs.hpp
src/python3/patch_level.hpp

Files skipped from review as they are similar to previous changes (21)

pyphare/pyphare/pharein/init.py
pyphare/pyphare/pharesee/hierarchy.py
src/amr/data/field/coarsening/field_coarsen_operator.hpp
src/amr/physical_models/hybrid_model.hpp
src/core/CMakeLists.txt
src/core/data/grid/gridlayoutimplyee.hpp
src/core/data/tensorfield/tensorfield.hpp
src/diagnostic/detail/types/electromag.hpp
src/diagnostic/detail/types/particle.hpp
src/diagnostic/diagnostics.hpp
src/phare_core.hpp
src/restarts/detail/h5writer.hpp
tests/core/data/ion_population/test_ion_population.cpp
tests/core/data/ions/test_ions.cpp
tests/core/numerics/ion_updater/test_updater.cpp
tests/diagnostic/test_diagnostics.hpp
tests/diagnostic/test_diagnostics.ipp
tests/simulator/test_diagnostic_timestamps.py
tests/simulator/test_diagnostics.py
tests/simulator/test_initialization.py
tests/simulator/test_tagging.py

Additional comments: 63

pyphare/pyphare/pharein/diagnostics.py (1)

237-238: The FluidDiagnostics_ class raises an error for the 'pressure_tensor' quantity, which suggests that this feature is not yet implemented. Ensure that this is communicated clearly in the PR description and documentation.

pyphare/pyphare/pharein/simulation.py (4)

813-813: Initialization of self.diagnostics as a dictionary is a significant change from a list and affects how diagnostics are managed.

872-875: The check for existing diagnostics by name before adding a new one ensures that there are no duplicates, which is a good practice when using a dictionary to store items by a unique key.

896-902: The updated count_diagnostics method correctly iterates over the dictionary items and counts diagnostics of a specific type. This change aligns with the new dictionary-based storage of diagnostics.

893-905: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [810-902]

The changes to the Simulation class are consistent with the PR's objective to generalize the ParticleToMesh operator and add new diagnostics capabilities. The modifications to self.diagnostics and related methods are logical and appear to be correctly implemented.

pyphare/pyphare/pharesee/run.py (4)

160-187: The functions _compute_pressure and _compute_pop_pressure should be reviewed for correctness in their mathematical computations and ensure that they are consistent with the physical models they represent.

352-395: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [335-373]

Review the new methods added to the Run class, such as GetMassDensity, GetPressure, and GetPi, to ensure they are correctly implemented and integrate well with the existing methods.

374-382: Verify that the updates to the GetJ and GetDivB methods in the Run class correctly utilize the new _compute_current and _compute_divB functions and that the results are consistent with the expected physical quantities.

384-393: Review the GetRanks method in the Run class to ensure that it correctly uses the new _get_rank function and that the results are consistent with the expected MPI rank distribution.

src/core/data/ions/ion_population/ion_population.hpp (7)

21-21: The addition of TensorField as a template parameter to IonPopulation class aligns with the PR's objective to generalize the computation of various quantities.

33-39: Initialization of momentumTensor_ in the constructor is correct and necessary for the new functionality.

52-62: The updates to isUsable and isSettable methods to include momentumTensor_ are correct and ensure that the new tensor field is properly managed.

191-195: The addition of accessor methods for momentumTensor_ is correct and allows external code to interact with the new tensor field.

211-211: The change from std::vector to std::array for MomentProperties and ParticleProperties may limit flexibility if the number of properties needs to change at runtime. Verify if this aligns with the intended use cases and ensure that the fixed size of 1 is appropriate.

261-263: The update to getCompileTimeResourcesUserList to include momentumTensor_ is correct and ensures that the new tensor field is considered as a compile-time resource.

286-286: The addition of the momentumTensor_ member to the IonPopulation class is consistent with the changes made to the public interface and the PR's objectives.

src/core/data/ions/ions.hpp (10)

10-11: The inclusion of <vector> and <array> headers is appropriate given the use of these data structures in the class.

32-32: The addition of tensorfield_type as a type alias is consistent with the introduction of momentum tensor calculations.

50-50: The initialization of momentumTensor_ with a name and quantity type is consistent with the new functionality for momentum tensor computation.

76-90: The conditional logic in massDensity() to return either rho_ or massDensity_ based on sameMasses_ is clear and maintains backward compatibility for cases where all populations have the same mass.

101-103: The addition of momentumTensor() methods for both const and non-const access is consistent with the existing pattern for other fields like density() and velocity().

212-220: The isUsable() method now correctly checks the usability of momentumTensor_ and includes logic to ignore massDensity_ when sameMasses_ is true, which aligns with the PR's objective.

229-237: The isSettable() method has been updated to include checks for momentumTensor_ and to conditionally check massDensity_ based on sameMasses_, which is consistent with the changes made to isUsable().

259-263: The getFieldNamesAndQuantities() method has been updated to return different fields based on whether all populations have the same mass, which is a logical extension of the new functionality.

294-294: The getCompileTimeResourcesUserList() method now includes momentumTensor_ in the tuple, which is necessary for the new momentum tensor functionality.

315-315: The sameMasses() method provides a public accessor for the sameMasses_ member, which could be useful for external checks on population mass uniformity.

src/core/data/vecfield/vecfield.hpp (3)

17-18: The aliasing of VecField to TensorField with a rank of 1 is a major change. Verify that all usages of VecField throughout the codebase are compatible with the new TensorField interface.

6-8: Confirm whether <algorithm> and <unordered_map> are now used in this file, or if they should be removed as previously suggested.

32-34: Consider making the members of VecFieldNames const if they are not intended to be modified after construction.

src/core/hybrid/hybrid_quantities.hpp (2)

16-37: The addition of new momentum tensor components to the Scalar enum and the introduction of the Tensor enum are significant changes. Ensure that all parts of the codebase that rely on these enums, especially those iterating over Scalar values or using the count value, have been updated to handle the new components correctly.

13-46: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [13-75]

Ensure that the changes made to the HybridQuantity class in hybrid_quantities.hpp are consistent with related changes in other files, as per the PR objectives. This includes updates to classes, methods, and tests that may depend on the new tensor fields and momentum tensor components.
src/core/numerics/interpolator/interpolator.hpp (3)
284-297: The refactoring of ParticleToMesh to use a generic function func is a significant change. Ensure that all usages of this class have been updated to pass the appropriate function and that the behavior remains consistent with the previous implementation.

472-500: The changes to the operator() in the Interpolator class use lambdas for density and flux calculations. Verify that these changes are consistent with the intended behavior and that all calls to this operator have been updated accordingly.

568-610: The addition of the MomentumTensorInterpolator class is a significant new feature. Ensure that this class is correctly integrated and that all necessary parts of the codebase are updated to support its usage.

Verification successful

The search for usages of the MomentumTensorInterpolator class in the codebase has found a reference in the file src/diagnostic/detail/types/fluid.hpp. This indicates that the new class is indeed being used within the codebase, which aligns with the changes described in the pull request summary and the diff for interpolator.hpp.

Based on the information provided, there are no indications of issues with the integration of the MomentumTensorInterpolator class. The code snippet from the review comment shows a proper definition of the class, and the search result confirms its usage.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for usages of MomentumTensorInterpolator class in the codebase.
rg --type cpp "MomentumTensorInterpolator<"
Length of output: 159
src/diagnostic/detail/h5writer.hpp (7)

38-45: Ensure that all references to the Writer class have been updated to H5Writer throughout the codebase.

57-65: Verify that all member variables of H5Writer are properly initialized in the constructor.

142-148: The method writeTensorFieldAsDataset correctly uses the rank of the TensorField to iterate over components.

166-169: The typeWriters_ map is initialized with string literals as keys, which is a common practice.

229-244: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [212-235]

The dump method has been updated to finalize diagnostics and set file flags after writing datasets, aligning with the PR objectives.

256-269: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [259-293]

The initializeDatasets_ and writeDatasets_ methods are used within dump and dump_level to manage the lifecycle of datasets, which is consistent with the class's responsibilities.

309-315: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [299-327]

The writeDatasets_ method has been updated to include logic for writing attributes after datasets, which is consistent with the PR objectives.

src/diagnostic/detail/types/fluid.hpp (9)

5-5: The inclusion of the interpolator header is consistent with the PR's objective to use interpolators for momentum tensor calculations.

46-46: The new compute method implementation is in line with the PR's goal to compute momentum tensors.

73-139: The lambda function within compute method correctly captures the necessary variables and iterates over ions to compute momentum tensors, aligning with the PR's objectives.

149-154: The createFiles method has been updated to include checks for the new momentum_tensor diagnostic, which is consistent with the PR's changes.

191-216: The getDataSetInfo method now includes logic to handle momentum tensor data, which is consistent with the PR's changes.

266-288: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [252-285]

The initDataSets method has been updated to handle the initialization of datasets for the new momentum tensor diagnostics, which is consistent with the PR's changes.

299-325: The write method has been updated to include writing of the new momentum tensor diagnostics, which is consistent with the PR's changes.

349-349: The writeAttributes method has been updated to handle the new momentum tensor diagnostics, which is consistent with the PR's changes.

58-162: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [2-328]

Overall, the changes in fluid.hpp align with the PR's objectives to compute pressure diagnostics and are consistent with the AI-generated overview.

tests/amr/models/test_models.cpp (1)

10-10: The inclusion of tensorfield.hpp is consistent with the PR's objectives to support new tensor field types.

tests/amr/resources_manager/test_resources_manager.cpp (3)

8-9: The inclusion of vecfield.hpp and tensorfield.hpp headers aligns with the introduction of the TensorField class template and its alias SymTensorField as mentioned in the PR's objective and AI-generated summary.

32-37: The updates to the type aliases to include SymTensorField1D as a template parameter are consistent with the PR's objective to enable the computation of pressure diagnostics.

1-12: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-40]

Please verify that the removal of include directives for "core/data/grid/gridlayout_impl.hpp" and "gmock/gmock.h" does not impact the functionality of the test file or any other parts of the codebase that may rely on these headers.

tests/core/data/electrons/test_electrons.cpp (6)

7-7: The inclusion of tensorfield.hpp aligns with the PR's objective to support tensor fields for pressure diagnostics.

129-135: The addition of SymTensorFieldND to IonPopulationND and IonsT types is necessary for the new momentum tensor functionality.

152-163: The addition of fields for momentum tensor components like Mxx, Mxy, Mxz, etc., is consistent with the PR's objective to compute pressure diagnostics.

235-241: The setting of momentumTensor buffers for ions is correctly implemented.

250-255: The setting of momentumTensor buffers for the first ion population (pops[0]) is correctly implemented.

1-1: The test setup and teardown methods correctly manage the lifecycle of the buffers, which is good practice to avoid dangling pointers.

tests/diagnostic/__init__.py (3)

29-32: The inclusion of "pressure_tensor" in the list of quantities for FluidDiagnostics is consistent with the PR's objective to compute pressure diagnostics.

38-41: The addition of "pressure_tensor" to the list of quantities for particle diagnostics in the loop iterating over pops is consistent with the PR's objective to compute pressure diagnostics for particle populations.

26-32: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-7]

Moving the numpy import statements to the top of the functions timestamps_with_step and all_timestamps is a good practice for clarity and performance, although it was not explicitly mentioned in the PR's objective or summaries.

coderabbitai · 2023-12-18T15:33:18Z

tests/core/data/gridlayout/test_laplacian.py

+            Jx[
+                psi_d_X + 1 : pei_d_X + 2 :,
+            ]
            - 2.0 * Jx[psi_d_X : pei_d_X + 1, :]
            + Jx[psi_d_X - 1 : pei_d_X, :]


There appears to be a missing slice for the second dimension in the calculation of Jx_x. The slice should be :, to include all elements along that axis.

- Jx[psi_d_X + 1 : pei_d_X + 2 :,] + Jx[psi_d_X + 1 : pei_d_X + 2, :,]

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.

Suggested change

Jx[

psi_d_X + 1 : pei_d_X + 2 :,

]

- 2.0 * Jx[psi_d_X : pei_d_X + 1, :]

+ Jx[psi_d_X - 1 : pei_d_X, :]

Jx[

psi_d_X + 1 : pei_d_X + 2, :,

]

- 2.0 * Jx[psi_d_X : pei_d_X + 1, :]

+ Jx[psi_d_X - 1 : pei_d_X, :]

coderabbitai · 2023-12-18T15:33:18Z

pyphare/pyphare/pharesee/run.py

+def _compute_current(patchdatas, **kwargs):
+    reference_pd = patchdatas["Bx"]  # take Bx as a reference, but could be any other
+
+    ndim = reference_pd.box.ndim
+    if ndim == 1:
+        By = patchdatas["By"].dataset[:]
+        xby = patchdatas["By"].x
+        Bz = patchdatas["Bz"].dataset[:]
+        xbz = patchdatas["Bz"].x
        Jy, Jz = _current1d(By, Bz, xby, xbz)
        return (
            {"name": "Jy", "data": Jy, "centering": "primal"},
            {"name": "Jz", "data": Jz, "centering": "primal"},
        )

-    elif patch.box.ndim == 2:
-        Bx = patch.patch_datas["Bx"].dataset[:]
-        By = patch.patch_datas["By"].dataset[:]
-        Bz = patch.patch_datas["Bz"].dataset[:]
+    elif ndim == 2:
+        Bx = patchdatas["Bx"].dataset[:]
+        By = patchdatas["By"].dataset[:]
+        Bz = patchdatas["Bz"].dataset[:]

-        dx, dy = patch.dl
+        dx, dy = reference_pd.dl

        Jx, Jy, Jz = _current2d(Bx, By, Bz, dx, dy)
-        # return ({"name":"Jx", "data":Jx,"centering":"primal"},
-        #        {"name":"Jy", "data":Jy,"centering":"primal"},
-        #        {"name":"Jz", "data":Jz,"centering":"primal"})

        components = ("Jx", "Jy", "Jz")
        centering = {
            component: [
-                patch.layout.centering[direction][component] for direction in ("X", "Y")
+                reference_pd.layout.centering[direction][component]
+                for direction in ("X", "Y")
            ]
            for component in components
        }


Ensure consistent return types for all branches within the _compute_current function to avoid any implicit returns that may return None.

coderabbitai · 2023-12-18T15:33:18Z

pyphare/pyphare/pharesee/run.py

+def _get_rank(patchdatas, **kwargs):
    """
    make a field dataset cell centered coding the MPI rank
    rank is obtained from patch global id == "rank#local_patch_id"
    """
    from pyphare.core.box import grow

-    layout = patch.layout
+    reference_pd = patchdatas["Bx"]  # take Bx as a reference, but could be any other
+    ndim = reference_pd.box.ndim
+    pid = kwargs["id"]
+
+    layout = reference_pd.layout
    centering = "dual"
    nbrGhosts = layout.nbrGhosts(layout.interp_order, centering)
-    shape = grow(patch.box, [nbrGhosts] * 2).shape
+    shape = grow(reference_pd.box, [nbrGhosts] * 2).shape

-    if patch.box.ndim == 1:
+    if ndim == 1:
        pass

-    if patch.box.ndim == 2:
-        data = np.zeros(shape) + int(patch.id.strip("p").split("#")[0])
+    elif ndim == 2:
+        data = np.zeros(shape) + int(pid.strip("p").split("#")[0])
        return ({"name": "rank", "data": data, "centering": [centering] * 2},)
    else:
        raise RuntimeError("Not Implemented yet")


Ensure consistent return types for all branches within the _get_rank function to avoid any implicit returns that may return None.

coderabbitai · 2023-12-18T15:33:19Z

tests/amr/models/test_models.cpp

+using SymTensorField1D                   = SymTensorField<NdArrayVector<1>, HybridQuantity>;
 using GridImplYee1D                      = GridLayoutImplYee<dim, interpOrder>;
 using ParticleArray1D                    = ParticleArray<dim>;
 using GridYee1D                          = GridLayout<GridImplYee1D>;
 using MaxwellianParticleInitializer1D = MaxwellianParticleInitializer<ParticleArray1D, GridYee1D>;
-using IonsPop1D                       = IonPopulation<ParticleArray1D, VecField1D, GridYee1D>;
-using Ions1D                          = Ions<IonsPop1D, GridYee1D>;
-using Electromag1D                    = Electromag<VecField1D>;
-using Electrons1D                     = Electrons<Ions1D>;
+using IonsPop1D         = IonPopulation<ParticleArray1D, VecField1D, SymTensorField1D, GridYee1D>;
+using Ions1D            = Ions<IonsPop1D, GridYee1D>;
+using Electromag1D      = Electromag<VecField1D>;
+using Electrons1D       = Electrons<Ions1D>;


💡 NOTE
Codebase verification is a beta feature.

The search results indicate that there are still instances in the codebase where VecField1D is used in conjunction with IonPopulation and Electromag, which may not be consistent with the recent changes made in the test_models.cpp file. Specifically, VecField1D is still being used in the type alias for IonPopulation in tests/amr/resources_manager/test_resources_manager.cpp and tests/core/data/ions/test_ions.cpp, as well as for Electromag in tests/amr/resources_manager/test_resources_manager.cpp and tests/amr/models/test_models.cpp.

Given this information, it seems that not all usages of these types have been updated to reflect the changes. Therefore, there may be inconsistencies in the codebase that require attention.

tests/amr/resources_manager/test_resources_manager.cpp still uses VecField1D with IonPopulation and Electromag.

tests/core/data/ions/test_ions.cpp still uses VecField1D with IonPopulation.

tests/amr/models/test_models.cpp still uses VecField1D with Electromag.

Analysis chain

The type aliases have been updated to use SymTensorField1D instead of VecField1D where appropriate. Ensure that all usages of these types in the codebase are updated to reflect these changes, especially in the context of IonPopulation and related classes.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash # Search for instances of the old `VecField1D` being used with `IonPopulation` and related classes. rg --type cpp 'IonPopulation<.*VecField1D.*>' rg --type cpp 'Ions<.*VecField1D.*>' rg --type cpp 'Electromag<.*VecField1D.*>' rg --type cpp 'Electrons<.*VecField1D.*>'

Length of output: 767

coderabbitai · 2023-12-18T15:33:19Z

src/core/data/vecfield/vecfield_component.hpp

+        template<std::size_t rank = 1>
+        static std::unordered_map<std::string, Component> const& componentMap()
+        {
+            if constexpr (rank == 1)
+                return vecComponentMap;
+            else if constexpr (rank == 2)
+                return tensComponentMap;
+            else
+                throw std::runtime_error("rank must be 1 or 2");


The componentMap method now throws a runtime error for ranks other than 1 or 2. Consider handling this case more gracefully or ensuring that the function is never called with an invalid rank to avoid potential runtime exceptions.

template<std::size_t rank = 1> static std::unordered_map<std::string, Component> const& componentMap() { if constexpr (rank == 1) return vecComponentMap; else if constexpr (rank == 2) return tensComponentMap; // Suggestion: Instead of throwing, return a default map or handle the error differently. else return defaultComponentMap; // or some other error handling mechanism }

The componentMap method now throws a runtime error for ranks other than 1 or 2. Consider handling this case more gracefully or ensuring that the function is never called with an invalid rank to avoid potential runtime exceptions.

template<std::size_t rank = 1> static std::unordered_map<std::string, Component> const& componentMap() { if constexpr (rank == 1) return vecComponentMap; else if constexpr (rank == 2) return tensComponentMap; // Suggestion: Instead of throwing, return a default map or handle the error differently. else // Implement an appropriate error handling mechanism here. }

coderabbitai · 2023-12-18T15:33:19Z

pyphare/pyphare/pharein/diagnostics.py

                f"{self.__class__.__name__,}.flush_every cannot be negative"
            )

-        if any(
-            [
-                self.quantity == diagnostic.quantity
-                for diagnostic in global_vars.sim.diagnostics
-            ]
-        ):
-            raise RuntimeError(
-                f"Error: Diagnostic ({kwargs['quantity']}) already registered"
-            )
-
        self.__extent = None
-        global_vars.sim.add_diagnostics(self)
+
+        # if a diag already is registered we just contactenate the timestamps
+        addIt = True
+        registered_diags = global_vars.sim.diagnostics
+        for diagname, diag in registered_diags.items():
+            if self.quantity == diag.quantity:
+                print(
+                    f"{diag.name} already registered {self.quantity}, merging timestamps"
+                )
+                my_times = self.write_timestamps
+                existing_times = diag.write_timestamps
+                new_times = np.concatenate((my_times, existing_times))
+                new_times.sort()
+                mask = np.ones(len(new_times), dtype=bool)
+                mask[1:] = (
+                    np.diff(new_times) > 1e-10
+                )  # assumed smaller than any realistic dt
+                global_vars.sim.diagnostics[diagname].write_timestamps = new_times[mask]
+                addIt = False
+                break  # there can be only one
+
+        if addIt:
+            global_vars.sim.add_diagnostics(self)


The logic for merging timestamps in the Diagnostics class could be simplified by using built-in numpy functions such as np.unique which can replace the manual creation of a mask and sorting.

- new_times.sort() - mask = np.ones(len(new_times), dtype=bool) - mask[1:] = ( - np.diff(new_times) > 1e-10 - ) # assumed smaller than any realistic dt - global_vars.sim.diagnostics[diagname].write_timestamps = new_times[mask] + unique_times = np.unique(new_times, axis=0) + global_vars.sim.diagnostics[diagname].write_timestamps = unique_times

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.

Suggested change

f"{self.__class__.__name__,}.flush_every cannot be negative"

)

if any(

[

self.quantity == diagnostic.quantity

for diagnostic in global_vars.sim.diagnostics

]

):

raise RuntimeError(

f"Error: Diagnostic ({kwargs['quantity']}) already registered"

)

self.__extent = None

global_vars.sim.add_diagnostics(self)

# if a diag already is registered we just contactenate the timestamps

addIt = True

registered_diags = global_vars.sim.diagnostics

for diagname, diag in registered_diags.items():

if self.quantity == diag.quantity:

print(

f"{diag.name} already registered {self.quantity}, merging timestamps"

)

my_times = self.write_timestamps

existing_times = diag.write_timestamps

new_times = np.concatenate((my_times, existing_times))

new_times.sort()

mask = np.ones(len(new_times), dtype=bool)

mask[1:] = (

np.diff(new_times) > 1e-10

) # assumed smaller than any realistic dt

global_vars.sim.diagnostics[diagname].write_timestamps = new_times[mask]

addIt = False

break # there can be only one

if addIt:

global_vars.sim.add_diagnostics(self)

f"{self.__class__.__name__,}.flush_every cannot be negative"

)

self.__extent = None

# if a diag already is registered we just contactenate the timestamps

addIt = True

registered_diags = global_vars.sim.diagnostics

for diagname, diag in registered_diags.items():

if self.quantity == diag.quantity:

print(

f"{diag.name} already registered {self.quantity}, merging timestamps"

)

my_times = self.write_timestamps

existing_times = diag.write_timestamps

new_times = np.concatenate((my_times, existing_times))

unique_times = np.unique(new_times, axis=0)

global_vars.sim.diagnostics[diagname].write_timestamps = unique_times

addIt = False

break # there can be only one

if addIt:

global_vars.sim.add_diagnostics(self)

coderabbitai · 2023-12-18T15:33:20Z

pyphare/pyphare/pharein/diagnostics.py

+
        if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
            error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(


The error message in the ElectromagDiagnostics class could be more specific by stating that the provided quantity is not a valid electromag diagnostics.

- error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join( + error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(

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.

Suggested change

if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:

error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(

if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:

error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(

coderabbitai · 2023-12-18T15:33:20Z

pyphare/pyphare/pharein/diagnostics.py

+        if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
            error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
-                FluidDiagnostics.fluid_quantities
+                FluidDiagnostics_.fluid_quantities
            )
            raise ValueError(error_msg.format(kwargs["quantity"]))
        elif kwargs["quantity"] == "flux" and kwargs["population_name"] == "ions":
            raise ValueError(
                "'flux' is only available for specific populations, try 'bulkVelocity"
            )
+        elif kwargs["quantity"] == "pressure_tensor":
+            raise ValueError("'pressure_tensor' is not available yet")


The error handling in FluidDiagnostics_ for invalid quantities could be improved by providing a more informative error message.

- error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join( + error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(

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.

Suggested change

if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:

error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(

FluidDiagnostics.fluid_quantities

FluidDiagnostics_.fluid_quantities

)

raise ValueError(error_msg.format(kwargs["quantity"]))

elif kwargs["quantity"] == "flux" and kwargs["population_name"] == "ions":

raise ValueError(

"'flux' is only available for specific populations, try 'bulkVelocity"

)

elif kwargs["quantity"] == "pressure_tensor":

raise ValueError("'pressure_tensor' is not available yet")

if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:

error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(

FluidDiagnostics_.fluid_quantities

)

raise ValueError(error_msg.format(kwargs["quantity"]))

elif kwargs["quantity"] == "flux" and kwargs["population_name"] == "ions":

raise ValueError(

"'flux' is only available for specific populations, try 'bulkVelocity"

)

elif kwargs["quantity"] == "pressure_tensor":

raise ValueError("'pressure_tensor' is not available yet")

coderabbitai

Review Status

Actionable comments generated: 7

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between c8f030d and 2391950.

Files selected for processing (40)

pyphare/pyphare/core/gridlayout.py (3 hunks)
pyphare/pyphare/pharein/init.py (1 hunks)
pyphare/pyphare/pharein/diagnostics.py (10 hunks)
pyphare/pyphare/pharein/simulation.py (4 hunks)
pyphare/pyphare/pharesee/hierarchy.py (7 hunks)
pyphare/pyphare/pharesee/run.py (6 hunks)
src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 hunks)
src/amr/physical_models/hybrid_model.hpp (1 hunks)
src/core/CMakeLists.txt (1 hunks)
src/core/data/grid/gridlayoutdefs.hpp (1 hunks)
src/core/data/grid/gridlayoutimplyee.hpp (4 hunks)
src/core/data/ions/ion_population/ion_population.hpp (8 hunks)
src/core/data/ions/ions.hpp (13 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield.hpp (2 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/core/hybrid/hybrid_quantities.hpp (2 hunks)
src/core/numerics/interpolator/interpolator.hpp (11 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
src/diagnostic/detail/types/particle.hpp (1 hunks)
src/diagnostic/diagnostics.hpp (1 hunks)
src/phare_core.hpp (3 hunks)
src/python3/patch_level.hpp (6 hunks)
src/restarts/detail/h5writer.hpp (1 hunks)
tests/amr/models/test_models.cpp (3 hunks)
tests/amr/resources_manager/test_resources_manager.cpp (3 hunks)
tests/core/data/electrons/test_electrons.cpp (6 hunks)
tests/core/data/gridlayout/test_laplacian.py (1 hunks)
tests/core/data/ion_population/test_ion_population.cpp (3 hunks)
tests/core/data/ions/test_ions.cpp (2 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
tests/diagnostic/init.py (2 hunks)
tests/diagnostic/test_diagnostics.hpp (2 hunks)
tests/diagnostic/test_diagnostics.ipp (1 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (1 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Files skipped from review due to trivial changes (3)

pyphare/pyphare/core/gridlayout.py
src/core/data/grid/gridlayoutdefs.hpp
src/diagnostic/detail/h5writer.hpp

Files skipped from review as they are similar to previous changes (27)

pyphare/pyphare/pharein/init.py
pyphare/pyphare/pharein/simulation.py
src/amr/data/field/coarsening/field_coarsen_operator.hpp
src/amr/physical_models/hybrid_model.hpp
src/core/CMakeLists.txt
src/core/data/grid/gridlayoutimplyee.hpp
src/core/data/vecfield/vecfield.hpp
src/core/hybrid/hybrid_quantities.hpp
src/diagnostic/detail/types/electromag.hpp
src/diagnostic/detail/types/particle.hpp
src/diagnostic/diagnostics.hpp
src/phare_core.hpp
src/python3/patch_level.hpp
src/restarts/detail/h5writer.hpp
tests/amr/resources_manager/test_resources_manager.cpp
tests/core/data/electrons/test_electrons.cpp
tests/core/data/gridlayout/test_laplacian.py
tests/core/data/ion_population/test_ion_population.cpp
tests/core/data/ions/test_ions.cpp
tests/core/numerics/ion_updater/test_updater.cpp
tests/diagnostic/init.py
tests/diagnostic/test_diagnostics.hpp
tests/diagnostic/test_diagnostics.ipp
tests/simulator/test_diagnostic_timestamps.py
tests/simulator/test_diagnostics.py
tests/simulator/test_initialization.py
tests/simulator/test_tagging.py

Additional comments: 59

pyphare/pyphare/pharesee/hierarchy.py (11)

11-11: The addition of the listify function is consistent with the PR's objective to handle multiple types of fields and should help in processing lists of hierarchies.

151-151: The updated error message now includes the field name, which provides more context to the user when an error occurs. This is an improvement in usability.

1160-1165: The addition of momentum tensor components to the field_qties dictionary aligns with the PR's objective to handle new field types like momentum tensors.

1217-1217: The function compute_hier_from_ has been renamed to compute_hier_from___, which seems to be a typo or an incomplete refactoring. Please verify the correct naming convention and ensure consistency across the codebase.

1258-1266: The function extract_patchdatas is a new addition that extracts patch data from a list of hierarchies for a specific patch at a level. This function is consistent with the PR's objectives and should facilitate the manipulation of patch data across different hierarchies.

1269-1275: The function new_patchdatas_from creates new patch data using a compute function and existing patch data. This is in line with the PR's goal to generalize the ParticleToMesh operator and should enable the addition of new computations for fields.

1278-1287: The function new_patches_from creates new patches using a compute function and existing hierarchies. This function is crucial for the PR's goal to generalize the ParticleToMesh operator and supports the addition of new field computations.

1291-1309: The function compute_hier_from has been updated to compute a new hierarchy using a callback function on all patch data of given hierarchies. This is a significant change that supports the PR's objectives. Ensure that the compatibility check and the use of listify are correctly implemented and necessary.

1313-1313: The function pop_name has been modified to split the basename differently. Verify that this change does not break any existing functionality that relies on the previous behavior.

1555-1560: The addition of momentum tensor components to the isFieldQty function is necessary for the new functionality introduced in the PR. This change allows the code to recognize the new tensor fields as valid quantities.

1753-1773: The function getPatch is a new utility function that returns patches containing a given point, indexed by level. This could be useful for debugging and testing. Ensure that the error handling for multiple patches found for a single point is adequate and consider if this utility function should be part of the public API or if it's intended for internal use only.

pyphare/pyphare/pharesee/run.py (10)

5-5: The import of compute_hier_from_ is not used and can be removed to clean up the code.

5-5: Removing the unused import compute_hier_from_ will also resolve the cyclic import issue.

67-98: Refactoring of _compute_current to use patchdatas aligns with the PR's objective to generalize the ParticleToMesh operator.

113-128: Refactoring of _compute_divB to use patchdatas is consistent with the changes made in _compute_current.

134-157: Refactoring of _get_rank to use patchdatas and the addition of pid from kwargs is consistent with the overall changes.

160-228: Addition of _compute_pressure and _compute_pop_pressure functions is likely part of the new functionality to compute pressure using velocity squared.

352-395: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [335-394]

Addition of GetMassDensity, GetPressure, and GetPi methods to the Run class aligns with the PR's objective to support more complex particle-to-mesh operations.

67-98: The refactored _compute_current function now has explicit return statements for both branches, resolving the issue of mixed implicit and explicit returns.

113-128: The refactored _compute_divB function now has explicit return statements for both branches, resolving the issue of mixed implicit and explicit returns.

134-157: The refactored _get_rank function now has explicit return statements for both branches, resolving the issue of mixed implicit and explicit returns.
src/core/data/ions/ion_population/ion_population.hpp (9)
6-24: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [21-30]

The introduction of TensorField as a type alias is consistent with the PR's objectives to generalize the ParticleToMesh operator.

37-37: The addition of momentumTensor_ to the IonPopulation class is a significant change that supports the deposition of various fields, such as momentum tensors. Ensure that the initialization and usage of this new member are consistent throughout the codebase.

54-55: The update to the isUsable method to include momentumTensor_.isUsable() is a necessary change to ensure that the new tensor field is in a usable state before operations are performed on it.

61-62: Similarly, the update to the isSettable method to include momentumTensor_.isSettable() ensures that the tensor field can be set up correctly.

194-195: The addition of getter methods for momentumTensor_ allows for read and write access to the momentum tensor, which is crucial for the new functionality introduced in the PR.

211-211: The change from std::vector to std::array for MomentProperties may have implications for how these properties are managed in the code. Verify that the fixed size of std::array is appropriate in all contexts where MomentProperties is used.

Verification successful

The verification process has confirmed that the change from std::vector to std::array for MomentProperties in the IonPopulation class is appropriate for its current usage within the class. There are no other usages of MomentProperties within the IonPopulation class file that could be negatively impacted by this change. The fixed size of the array is consistent with the hardcoded return value in the getFieldNamesAndQuantities method, and there are no indications of a need for dynamic resizing that would necessitate a std::vector.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for usages of MomentProperties to ensure the fixed size array does not cause issues.
rg --type cpp "MomentProperties"
Length of output: 464

Script:
#!/bin/bash
# Check the implementation of IonPopulation::getFieldNamesAndQuantities
ast-grep --lang cpp --pattern $'class IonPopulation {
  $$$
  MomentProperties getFieldNamesAndQuantities() const {
    $$$
  }
  $$$
}'
Length of output: 482

Script:
#!/bin/bash
# Use ripgrep to find the implementation of getFieldNamesAndQuantities in IonPopulation
rg --type cpp --context 10 "MomentProperties getFieldNamesAndQuantities const" src/core/data/ions/ion_population/ion_population.hpp
Length of output: 584

Script:
#!/bin/bash
# Search for any other usages of MomentProperties within the IonPopulation class
rg --type cpp "MomentProperties" src/core/data/ions/ion_population/ion_population.hpp
Length of output: 221
* 229-229: The same verification is needed for `ParticleProperties` as it has also been changed from `std::vector` to `std::array`.

261-264: The update to getCompileTimeResourcesUserList to include momentumTensor_ is consistent with the addition of the momentum tensor to the IonPopulation class.

286-286: The declaration of momentumTensor_ in the private section of the IonPopulation class is a key part of the changes and should be carefully integrated with the rest of the class's functionality.
src/core/data/ions/ions.hpp (10)

50-50:
The addition of momentumTensor_ as a member variable is consistent with the PR's objective to generalize the ParticleToMesh operator.

76-80:
The conditional logic in massDensity() to return either rho_ or massDensity_ based on sameMasses_ is a good use of encapsulation and maintains backward compatibility.

101-103:
The addition of const and non-const versions of momentumTensor() is a standard practice for providing read and write access to member variables.

184-199:
The computeFullMomentumTensor() method correctly accumulates momentum tensor contributions from each population. This aligns with the PR's objective and the previous comment about adding an explanatory comment has been addressed.

212-216:
The update to isUsable() to include momentumTensor_.isUsable() is necessary for the new tensor field functionality.

229-233:
The update to isSettable() to include momentumTensor_.isSettable() is necessary for the new tensor field functionality.

259-263:
The conditional logic in getFieldNamesAndQuantities() to return different values based on sameMasses_ is a good use of encapsulation and maintains backward compatibility.

294-294:
The inclusion of momentumTensor_ in getCompileTimeResourcesUserList() is consistent with the changes and ensures that the new tensor field is considered as a resource.

315-315:
The addition of sameMasses() as a member function provides a clean API for querying the state of sameMasses_.

338-338:
The placement of momentumTensor_ at the end of the class member declarations is consistent with the existing structure. The previous comment about grouping related members together has been addressed by the addition of the sameMasses() method.

src/core/data/vecfield/vecfield_component.hpp (3)

35-40: The update to the check method correctly includes checks for the new tensor components.

43-47: The inline definitions of vecComponentMap and tensComponentMap are correctly initialized with their respective components.

49-56: The introduction of VectorComponents and TensorComponents provides a clean and clear way to access the vector and tensor component maps, respectively.

src/core/numerics/interpolator/interpolator.hpp (8)

12-14: The addition of new includes for hybrid_quantities.hpp, gridlayoutdefs.hpp, and range.hpp is consistent with the refactoring and enhancement of the codebase to support more complex particle-to-mesh operations.

284-300: The refactoring of the ParticleToMesh<1> class to use a generic function func for depositing values into the field is consistent with the PR's objective to generalize the operator. Ensure that the func passed to this operator is always valid and that the lambda functions used in the PR are thoroughly tested.

306-321: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [309-330]

The changes in the ParticleToMesh<2> class follow the same pattern as the 1D specialization, using a generic function func for depositing values. This is a good application of the DRY (Don't Repeat Yourself) principle, as it reduces code duplication by using a common interface for different dimensions.

337-352: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [337-364]

The ParticleToMesh<3> class has been updated to use a generic function func for depositing values, similar to the 1D and 2D specializations. This consistency across dimensions is beneficial for maintainability and future enhancements.

377-377: The addition of a protected section in the Interpolator class is a common practice in object-oriented design to facilitate code reuse and is consistent with the introduction of the MomentumTensorInterpolator derived class.

472-503: The Interpolator class now uses the particleToMesh_ method with lambda functions to deposit density and flux values, which aligns with the PR's objective to generalize the deposition of various fields.

551-551: The protected section added to the Interpolator class is a design choice that allows derived classes to access common functionality or data members, which is a positive change for code reuse and maintainability.

568-610: The addition of the MomentumTensorInterpolator class is in line with the PR's objective to allow for the deposition of various fields, including the momentum tensor. This class extends the Interpolator class and uses lambda functions to define the computation per particle, which is a flexible and extensible approach.

src/diagnostic/detail/types/fluid.hpp (5)

5-5: The addition of the interpolator header aligns with the PR's objective to enhance particle-to-mesh operations.

73-139: The refactoring of the compute method to include momentum tensor calculations is comprehensive and well-integrated with the class's functionality.

184-224: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [141-285]

The updates to data writing and initialization methods to support momentum tensor computations are consistent with the PR's objectives.

296-325: The updates to the write method to include momentum tensor data writing are correctly implemented and align with the PR's objectives.

346-349: The updates to the writeAttributes method to handle momentum tensor attributes are correctly implemented.
tests/amr/models/test_models.cpp (3)
43-43: The usage of VecField1D in Electromag1D may be inconsistent with the changes made to IonsPop1D. If Electromag is supposed to use the new SymTensorField1D, this needs to be updated.

41-41: The type alias for IonsPop1D includes both VecField1D and SymTensorField1D. Ensure that this is intentional and consistent with the intended use of these types in the codebase.

Verification successful

The usage of both VecField1D and SymTensorField1D in the IonsPop1D type alias is intentional and consistent across multiple test files in the codebase. There is no indication from the search results that this pattern is incorrect. Therefore, the inclusion of both types in the IonsPop1D alias appears to be correct as per the current codebase context.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for instances of `IonPopulation` being instantiated with both `VecField1D` and `SymTensorField1D` to verify if this is correct.
rg --type cpp 'IonPopulation<.*VecField1D,.*SymTensorField1D.*>'
Length of output: 471
* 33-47: The test file includes updates to type aliases to reflect the new data structures and logic. Ensure that the test cases within this file and other related test files are updated accordingly to use the new `SymTensorField1D` and other related types.

Verification successful

The search for test cases using the new SymTensorField1D type did not yield any results. This could mean that there are no explicit test cases named with SymTensorField1D in their definition, or that the test cases do not need to reference the type by name, possibly because they use variables or functions that have been updated to the new type.

Given that the search was quite specific and might have missed occurrences where SymTensorField1D is used without being in the test case name, it would be prudent to perform a broader search. We should look for any usage of SymTensorField1D and related types within the test files to ensure that the test cases are indeed using the updated types where necessary.

Let's execute a broader search to check for the usage of SymTensorField1D and related types in the test files.

The search within the test files for SymTensorField1D has produced several results, indicating that the type is indeed being used in various test files. The files test_models.cpp, test_resources_manager.cpp, and test_ions.cpp all contain references to SymTensorField1D, suggesting that the test cases in these files have been updated to reflect the new data structures introduced by the pull request.

Given this information, it appears that the necessary updates to the test cases to accommodate the new SymTensorField1D type and related changes have been made. Therefore, the verification for the updates to the test files concerning the use of SymTensorField1D is successful.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for test cases that might need to be updated to use the new `SymTensorField1D` and related types.
rg --type cpp 'TEST.*SymTensorField1D'
Length of output: 40

Script:
#!/bin/bash
# Search for any usage of `SymTensorField1D` and related types within the test files.
rg --type cpp 'SymTensorField1D' tests/
Length of output: 814

coderabbitai · 2023-12-18T16:23:44Z

pyphare/pyphare/pharesee/run.py

+def _compute_divB(patchdatas, **kwargs):
+
+    reference_pd = patchdatas["Bx"]  # take Bx as a reference, but could be any other
+    ndim = reference_pd.box.ndim
+
+    if ndim == 1:
        raise ValueError("divB is 0 by construction in 1D")

-    elif patch.box.ndim == 2:
-        By = patch.patch_datas["By"].dataset[:]
-        Bx = patch.patch_datas["Bx"].dataset[:]
-        xBx = patch.patch_datas["Bx"].x
-        yBy = patch.patch_datas["By"].y
+    elif ndim == 2:
+        By = patchdatas["By"].dataset[:]
+        Bx = patchdatas["Bx"].dataset[:]
+        xBx = patchdatas["Bx"].x
+        yBy = patchdatas["By"].y
        divB = _divB2D(Bx, By, xBx, yBy)

        return ({"name": "divB", "data": divB, "centering": ["dual", "dual"]},)


Consider running the black formatter on the file to ensure consistent code style.

coderabbitai · 2023-12-18T16:23:44Z

src/core/data/tensorfield/tensorfield.hpp

+    constexpr static std::size_t dimFromRank()
+    {
+        if constexpr (rank_ == 1) // Vector field
+            return 3;
+        else if constexpr (rank_ == 2) // symmetric 3x3 tensor field
+            return 6;


The dimFromRank function does not handle cases where rank_ is not 1 or 2. This could lead to undefined behavior if rank_ is set to a different value. Consider adding a default return value or handling additional cases.

constexpr static std::size_t dimFromRank() { if constexpr (rank_ == 1) // Vector field return 3; else if constexpr (rank_ == 2) // symmetric 3x3 tensor field return 6; else static_assert(rank_ == 1 || rank_ == 2, "Unsupported rank for TensorField"); }

coderabbitai

Review Status

Actionable comments generated: 14

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between c8f030d and bec9a17.

Files selected for processing (40)

pyphare/pyphare/core/gridlayout.py (3 hunks)
pyphare/pyphare/pharein/init.py (1 hunks)
pyphare/pyphare/pharein/diagnostics.py (10 hunks)
pyphare/pyphare/pharein/simulation.py (4 hunks)
pyphare/pyphare/pharesee/hierarchy.py (7 hunks)
pyphare/pyphare/pharesee/run.py (6 hunks)
src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 hunks)
src/amr/physical_models/hybrid_model.hpp (1 hunks)
src/core/CMakeLists.txt (1 hunks)
src/core/data/grid/gridlayoutdefs.hpp (1 hunks)
src/core/data/grid/gridlayoutimplyee.hpp (4 hunks)
src/core/data/ions/ion_population/ion_population.hpp (8 hunks)
src/core/data/ions/ions.hpp (13 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield.hpp (2 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/core/hybrid/hybrid_quantities.hpp (2 hunks)
src/core/numerics/interpolator/interpolator.hpp (11 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
src/diagnostic/detail/types/particle.hpp (1 hunks)
src/diagnostic/diagnostics.hpp (1 hunks)
src/phare_core.hpp (3 hunks)
src/python3/patch_level.hpp (6 hunks)
src/restarts/detail/h5writer.hpp (1 hunks)
tests/amr/models/test_models.cpp (3 hunks)
tests/amr/resources_manager/test_resources_manager.cpp (3 hunks)
tests/core/data/electrons/test_electrons.cpp (6 hunks)
tests/core/data/gridlayout/test_laplacian.py (1 hunks)
tests/core/data/ion_population/test_ion_population.cpp (3 hunks)
tests/core/data/ions/test_ions.cpp (2 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
tests/diagnostic/init.py (2 hunks)
tests/diagnostic/test_diagnostics.hpp (2 hunks)
tests/diagnostic/test_diagnostics.ipp (1 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (2 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Files skipped from review due to trivial changes (4)

src/amr/physical_models/hybrid_model.hpp
src/core/CMakeLists.txt
src/core/data/grid/gridlayoutdefs.hpp
src/core/data/grid/gridlayoutimplyee.hpp

Files skipped from review as they are similar to previous changes (23)

pyphare/pyphare/core/gridlayout.py
pyphare/pyphare/pharein/init.py
pyphare/pyphare/pharein/simulation.py
src/amr/data/field/coarsening/field_coarsen_operator.hpp
src/core/numerics/interpolator/interpolator.hpp
src/diagnostic/detail/types/electromag.hpp
src/diagnostic/detail/types/particle.hpp
src/diagnostic/diagnostics.hpp
src/python3/patch_level.hpp
src/restarts/detail/h5writer.hpp
tests/amr/resources_manager/test_resources_manager.cpp
tests/core/data/electrons/test_electrons.cpp
tests/core/data/gridlayout/test_laplacian.py
tests/core/data/ion_population/test_ion_population.cpp
tests/core/data/ions/test_ions.cpp
tests/core/numerics/ion_updater/test_updater.cpp
tests/diagnostic/init.py
tests/diagnostic/test_diagnostics.hpp
tests/diagnostic/test_diagnostics.ipp
tests/simulator/test_diagnostic_timestamps.py
tests/simulator/test_diagnostics.py
tests/simulator/test_initialization.py
tests/simulator/test_tagging.py

Additional comments: 58

pyphare/pyphare/pharesee/hierarchy.py (9)

11-11: The listify function has been added. Verify that it is being used correctly wherever lists of hierarchies are expected.

151-151: The updated error message in FieldData.__init__ is more informative as it includes the field name, which is helpful for debugging.

1217-1217: The compute_hier_from_ function is limited to 1D as per the comment, but the function name has been changed. Ensure that all references to this function are updated accordingly.

1258-1266: The extract_patchdatas function extracts patch data from a list of hierarchies. Ensure that the extracted data is correctly handled and that the function is called with the correct parameters.

1269-1275: The new_patchdatas_from function creates new patch data using a compute function. Verify that the compute function is provided and that it returns data in the expected format.

1278-1287: The new_patches_from function creates new patches using a compute function and existing hierarchies. Ensure that the compute function is correctly applied to each patch and that the new patches are consistent with the existing hierarchy structure.

1291-1309: The compute_hier_from function has been added to compute a new hierarchy using a callback function. Verify that the callback function is correctly implemented and that the new hierarchy is consistent with the existing hierarchies.

1555-1560: New field quantities related to the momentum tensor have been added to the isFieldQty function. Ensure that these new quantities are handled consistently throughout the codebase and that any associated computations are correctly implemented.

1753-1773: The getPatch function has been added to retrieve patches containing a given point. Verify that the function correctly identifies the patches and handles cases where multiple patches may contain the point.
pyphare/pyphare/pharesee/run.py (6)
67-98: The signature of _compute_current has been changed. Verify that all calls to this function have been updated to pass patchdatas instead of patch.

113-128: The signature of _compute_divB has been changed. Verify that all calls to this function have been updated to pass patchdatas instead of patch.

Verification successful

The verification process did not find any calls to _compute_divB in the codebase. This could mean that the function is not called from anywhere else, or that the calls have been updated in line with the changes to the function's signature. Given the absence of evidence to the contrary, it seems reasonable to conclude that the function's signature change has been handled correctly.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for calls to `_compute_divB` and verify that they pass `patchdatas`.
ast-grep --lang python --pattern $'_compute_divB($_)'
Length of output: 54
* 134-157: The signature of `_get_rank` has been changed. Verify that all calls to this function have been updated to pass `patchdatas` instead of `patch`.

Verification successful

The executed script did not return any results, which means that no calls to _get_rank were found in the codebase. This could imply that the function is not called anywhere, or that the function is called in a way that was not captured by the ast-grep pattern used.

Given that the function signature has changed, it is important to ensure that any existing calls to this function are updated accordingly. Since the script did not find any calls, there are two possibilities:

The function is indeed not called anywhere, and therefore there are no changes needed.

The function is called, but the search pattern was not able to find it due to limitations in the pattern or the complexity of the codebase.

To be thorough, I will generate a new shell script to search for the function name within the codebase using a different approach, which might capture the usage if it exists.

The attempt to use rg with the --python flag resulted in an error because --python is not a valid option for ripgrep. The correct way to search for Python files is to use the -t option followed by py. I will correct the script and run it again to search for occurrences of _get_rank within Python files in the codebase.

The output from the script indicates that the _get_rank function is defined in pyphare/pyphare/pharesee/run.py and is called in the same file. The call to _get_rank is made within a call to compute_hier_from, which suggests that it is being used as a callback or a function pointer.

Given that the definition of _get_rank has been updated to accept patchdatas instead of patch, we need to ensure that the call to this function is consistent with the new signature. The output shows that compute_hier_from is called with _get_rank and B, but without additional context, it is not clear what B represents and whether it is compatible with the new function signature.

To verify the compatibility, we need to check the implementation of compute_hier_from to understand how it calls _get_rank and what arguments it passes. I will generate a script to extract the relevant portion of the code where compute_hier_from is defined or called.

The output from the script provides context around the usage of compute_hier_from within the pyphare/pyphare/pharesee/run.py file. It shows that compute_hier_from is used to create a new hierarchy by applying a computation to each patch of the given hierarchies. The function takes a callback compute and a list of hierarchies as arguments, along with additional keyword arguments.

The call to compute_hier_from(_get_rank, B) indicates that _get_rank is used as the computation callback and B is passed as the hierarchy. Since B is used as the argument, it is likely that B represents a collection of patch data that is compatible with the new signature of _get_rank, which now expects patchdatas.

Given this information, it seems that the call to _get_rank within compute_hier_from is consistent with the updated function signature, as the function is designed to work with patch data collections, and B appears to be such a collection.

Based on the information gathered, the change in the signature of _get_rank to accept patchdatas instead of patch is consistent with its usage in the codebase, and no further action is required to update calls to this function.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for calls to `_get_rank` and verify that they pass `patchdatas`.
ast-grep --lang python --pattern $'_get_rank($_)'
Length of output: 50

Script:
#!/bin/bash
# Use ripgrep to search for "_get_rank" to ensure we're not missing any calls due to complex code patterns.
rg '_get_rank' --python
Length of output: 434

Script:
#!/bin/bash
# Correct the ripgrep command to search for "_get_rank" in Python files.
rg '_get_rank' -tpy
Length of output: 168

Script:
#!/bin/bash
# Extract the relevant code around the call to `compute_hier_from` to understand how `_get_rank` is used.
rg -C 10 'compute_hier_from' -tpy
Length of output: 7900
* 160-187: Ensure that the new `_compute_pressure` function is called correctly with the appropriate arguments and that its implementation is consistent with the intended calculations.

189-227: Ensure that the new _compute_pop_pressure function is called correctly with the appropriate arguments and that its implementation is consistent with the intended calculations.

352-395: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [335-373]

New methods GetMassDensity, GetPressure, and GetPi have been added to the Run class. Ensure that they are correctly implemented and that their integration with the existing codebase does not introduce any issues.
src/core/data/ions/ion_population/ion_population.hpp (9)

30-30: The introduction of tensorfield_type as an alias for TensorField is consistent with the existing pattern of type aliases in the class and supports the generalization of the ParticleToMesh operator.

37-37: Initialization of momentumTensor_ in the constructor is correct and follows the naming convention used for other members like flux_.

54-55: The update to the isUsable method to include momentumTensor_.isUsable() is necessary for the new tensor field functionality and ensures that the momentumTensor_ is in a usable state before operations are performed on it.

61-62: Similarly, the update to the isSettable method to include momentumTensor_.isSettable() is correct and ensures that the momentumTensor_ can be set when needed.

194-195: The addition of momentumTensor accessor methods is consistent with the existing pattern of providing access to member variables and is necessary for external manipulation of the momentumTensor_.

211-211: The change from std::vector to std::array for MomentProperties suggests an optimization for a fixed-size collection, which is more performant due to stack allocation and better cache locality. However, ensure that this change aligns with the intended usage and that a fixed size of 1 is appropriate for all use cases.

229-229: Similarly, the change to ParticleProperties should be verified for correctness in the context of the intended usage, as it now assumes there will always be exactly one ParticleProperty.

261-263: The update to getCompileTimeResourcesUserList to include momentumTensor_ is necessary for the new functionality and ensures that the momentumTensor_ is considered when compiling resources.

286-286: The addition of the momentumTensor_ member variable to the private section of the class is consistent with the encapsulation of other member variables and is necessary for the new functionality.

src/core/data/ions/ions.hpp (11)

10-11: The inclusion of <vector> and <array> headers is appropriate for the use of these types in the Ions class.

32-32: The introduction of tensorfield_type as an alias is consistent with the PR objectives to handle tensor fields.

50-50: The addition of momentumTensor_ as a member variable is in line with the PR objectives to compute momentum tensors.

76-90: The massDensity method has been overloaded to provide both const and non-const access. This is a common C++ pattern and is correctly implemented here.

101-103: The momentumTensor method has been overloaded to provide both const and non-const access, which is consistent with the changes to massDensity.

212-216: The modification to the isUsable method to include momentumTensor_.isUsable() is correct and ensures that the momentum tensor is considered when determining if the Ions object is usable.

229-233: The modification to the isSettable method to include momentumTensor_.isSettable() is correct and ensures that the momentum tensor is considered when determining if the Ions object is settable.

259-263: The getFieldNamesAndQuantities method has been modified to return different values based on sameMasses_. This change is consistent with the PR objectives and is correctly implemented.

276-276: The assertion assert(sameMasses_ == false) ensures that massDensity_ is only set when the populations have different masses. This is a good use of assertions to enforce class invariants.

294-294: The getCompileTimeResourcesUserList method now includes momentumTensor_, which is consistent with the other changes to include momentum tensor calculations.

315-315: The addition of the sameMasses method provides a way to query the state of sameMasses_, which is useful for external code that needs to know if all populations have the same mass.

src/core/data/vecfield/vecfield.hpp (2)

17-18: The alias for VecField has been updated to use TensorField with a rank of 1, which is consistent with the PR's objective to generalize the ParticleToMesh operator.

32-34: Ensure that the VecFieldNames struct is compatible with the new VecField alias and that the construction of VecFieldNames from a VecField object is still valid.

src/core/data/vecfield/vecfield_component.hpp (3)

38-40: The check method has been updated to include checks for the new tensor components, which is necessary for the new functionality.

43-47: The vecComponentMap and tensComponentMap are defined inline with the correct mappings for vector and tensor components, which is necessary for the new functionality.

51-56: The VectorComponents and TensorComponents structs provide static methods to access the appropriate component maps, aligning with the PR's objective to generalize the ParticleToMesh operator.

src/core/hybrid/hybrid_quantities.hpp (2)

16-38: The extension of the Scalar enum class to include momentum tensor components is consistent with the PR's objectives to support more complex particle-to-mesh operations.

40-43: Introduction of the Tensor enum class and the TensorType template alias is a good approach to handle different ranks of tensor fields, facilitating the generalization of the ParticleToMesh operator.
src/diagnostic/detail/h5writer.hpp (1)
26-45: The class template Writer has been renamed to H5Writer. Verify that all references to the Writer class throughout the codebase have been updated to H5Writer.

Verification successful

The search for the old class name Writer did not produce any output, which suggests that all references to the Writer class may have been updated to H5Writer as intended. However, to be thorough, I will perform an additional check for any lingering references to Writer without the template keyword, as they might still be present in the codebase.

The search results indicate that there are still references to a Writer class or type in various files, particularly within the src/restarts and src/diagnostic directories. However, it is important to note that these references may not necessarily be related to the Writer class that was renamed to H5Writer in the src/diagnostic/detail/h5writer.hpp file. The term Writer could be used in a template parameter or in a different context.

Given that the search results include references to Writer in template parameters and class definitions, it is likely that these are generic or different uses of the term Writer and not directly related to the renamed H5Writer class. To confirm this, we would need to check the context of each occurrence to ensure they are not mistakenly referring to the old Writer class that has been renamed.

However, since the search for the specific template class Writer did not yield results, it is reasonable to conclude that the renaming of the Writer class to H5Writer within the context of the src/diagnostic/detail/h5writer.hpp file has been handled correctly.

Based on the information available, it appears that the renaming has been done appropriately within the context of the h5writer.hpp file, and the remaining references to Writer are likely unrelated to this specific renaming.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for the old class name "Writer" to ensure it's been replaced everywhere necessary.
rg --type cpp "template\s*<\s*typename\s*[\w\s]+>\s*class\s*Writer"
Length of output: 69

Script:
#!/bin/bash
# Search for any lingering references to "Writer" without the "template" keyword.
rg --type cpp "\bWriter\b"
Length of output: 3473
src/diagnostic/detail/types/fluid.hpp (6)

5-5: The addition of the interpolator header is justified by its usage in the compute function.

46-46: The compute function has been updated to include momentum tensor computations, aligning with the PR's objective to generalize the ParticleToMesh operator.

299-325: The write function has been updated to handle the writing of momentum tensor data, supporting the new tensor field data structures.

346-349: The writeAttributes function has been updated to include momentum tensor attributes, ensuring the new data structures are correctly described.

266-288: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [252-285]

The initDataSets function has been updated to initialize datasets for the momentum tensor, which is necessary for the new functionality.

191-216: The getDataSetInfo function has been updated to include information about the momentum tensor, ensuring that the metadata for the new tensor fields is correctly captured.

src/phare_core.hpp (4)

33-45: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [36-55]

The changes to the type declarations, including the introduction of SymTensorField_t and its use in IonPopulation_t, align with the PR's objective to generalize the ParticleToMesh operator. This refactoring appears to support the deposition of various fields, such as density, flux, and pressure, as mentioned in the PR objectives.

54-55: The update to the Electrons_t type to use Ions_t as a template parameter is consistent with the changes made to the IonPopulation_t and supports the generalization of the ParticleToMesh operator.

38-38: The VecField_t type remains unchanged, which is appropriate as the PR's objective is to generalize the ParticleToMesh operator, not to replace existing types but to add new capabilities alongside them.

36-42: The foundational types such as Array_t, Field_t, Electromag_t, YeeLayout_t, and GridLayout_t remain unchanged, indicating that they continue to serve their purpose in the context of the new generalization.
tests/amr/models/test_models.cpp (5)
10-10: The inclusion of tensorfield.hpp aligns with the PR's objective to support more complex particle-to-mesh operations.

36-36: The introduction of SymTensorField1D is consistent with the PR's goal to generalize the ParticleToMesh operator.

41-41: The update to IonsPop1D to include SymTensorField1D is in line with the PR's objectives. Ensure that all instances where IonsPop1D is used are updated accordingly.

Verification successful

Based on the output from the shell script, there are only two instances of IonsPop1D in the test_models.cpp file. Both instances have been updated to include SymTensorField1D as part of their type declaration. This is consistent with the PR's objectives to generalize the ParticleToMesh operator and to accommodate the deposition of various fields, such as density, flux, and pressure.

Since the search was limited to the test_models.cpp file and the PR's scope includes changes across various files, it's not possible to confirm that all instances across the entire codebase have been updated without performing a broader search. However, within the context of this file, the usage of IonsPop1D appears to be correctly updated.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for instances of `IonsPop1D` to ensure they are updated to include `SymTensorField1D`.
rg --type cpp 'IonsPop1D'
Length of output: 244
* 43-43: The `Electromag1D` type alias still uses `VecField1D`. Confirm whether this is intentional or if it should also be updated to use `SymTensorField1D`.

Verification successful

The search results show that Electromag1D is consistently defined as Electromag<VecField1D> across multiple files in the tests/amr directory. This suggests that the use of VecField1D in the type alias Electromag1D is consistent with other parts of the test suite and is likely intentional.

Given this information, it seems that the use of VecField1D in the Electromag1D type alias is correct and does not need to be updated to SymTensorField1D in the context of the test files. The SymTensorField1D type is used in the IonsPop1D type alias, which is separate from the Electromag1D type alias.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for instances of `Electromag1D` to verify if `VecField1D` should be replaced with `SymTensorField1D`.
rg --type cpp 'Electromag1D'
Length of output: 974
* 36-44: Considering the previous comments from the bot, verify if the issues with `VecField1D` usage have been resolved in this PR, particularly in the mentioned files.

coderabbitai · 2023-12-18T18:28:37Z

src/core/hybrid/hybrid_quantities.hpp

+    static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty)
+    {
+        // no condition, for now there's only then momentum tensor M
+        return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}};
+    }


The componentsQuantities function for Tensor types currently only handles the momentum tensor M. To future-proof this method, consider adding a condition to handle additional tensor quantities that may be added later.

static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty) { + if (qty == Tensor::M) return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}}; + else + throw std::runtime_error("Error - invalid Tensor"); }

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.

Suggested change

static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty)

{

// no condition, for now there's only then momentum tensor M

return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}};

}

static constexpr std::array<Scalar, 6> componentsQuantities(Tensor qty)

{

if (qty == Tensor::M)

return {{Scalar::Mxx, Scalar::Mxy, Scalar::Mxz, Scalar::Myy, Scalar::Myz, Scalar::Mzz}};

else

throw std::runtime_error("Error - invalid Tensor");

}

coderabbitai · 2023-12-18T18:28:37Z

src/phare_core.hpp

 #ifndef PHARE_CORE_INCLUDE_HPP
 #define PHARE_CORE_INCLUDE_HPP



Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [11-11]

Duplicate include detected for "core/models/physical_state.hpp". This should be removed to maintain code cleanliness and adhere to the DRY principle.

- #include "core/models/physical_state.hpp"

coderabbitai · 2023-12-18T18:28:37Z

src/core/data/vecfield/vecfield_component.hpp

+        template<std::size_t rank = 1>
+        static std::unordered_map<std::string, Component> const& componentMap()
+        {
+            if constexpr (rank == 1)
+                return vecComponentMap;
+            else if constexpr (rank == 2)
+                return tensComponentMap;
+            else
+                throw std::runtime_error("rank must be 1 or 2");
+        }


The componentMap function now handles different ranks and throws a runtime error for ranks other than 1 or 2. This is a good approach to separate the logic for vector and tensor components. However, consider handling this case more gracefully or ensuring that the function is never called with an invalid rank.

coderabbitai · 2023-12-18T18:28:37Z

src/core/data/vecfield/vecfield_component.hpp

        template<typename T>
        static Component at(T t)
        {
-            return componentMap.at(std::forward<T>(t));
+            return componentMap().at(std::forward<T>(t));


The at method defaults to using componentMap() with rank 1. Ensure that this method is only used in contexts where the rank is indeed 1, or consider templating this method as well to specify the rank.

coderabbitai · 2023-12-18T18:28:38Z

pyphare/pyphare/pharesee/hierarchy.py

+def are_compatible_hierarchies(hierarchies):
+    return True


The are_compatible_hierarchies function always returns True, which suggests it does not perform any actual compatibility check. This could lead to issues if incompatible hierarchies are assumed to be compatible. Implement the necessary logic or document this as a placeholder if future implementation is intended.

def are_compatible_hierarchies(hierarchies): # TODO: Implement actual compatibility logic or document as a placeholder raise NotImplementedError("Compatibility check not implemented yet.")

coderabbitai · 2023-12-18T18:28:38Z

src/diagnostic/detail/types/fluid.hpp

+private:
+    auto isActiveDiag(DiagnosticProperties const& diagnostic, std::string const& tree,
+                      std::string var)
+    {
+        return diagnostic.quantity == tree + var;
+    };


Consider refactoring isActiveDiag into a utility function if it's common across different diagnostics.

coderabbitai · 2023-12-18T18:28:39Z

src/diagnostic/detail/types/fluid.hpp

+template<typename H5Writer>
+void FluidDiagnosticWriter<H5Writer>::compute(DiagnosticProperties& diagnostic)
+{
+    core::MomentumTensorInterpolator<dimension, interp_order> interpolator;
+
+    auto& h5Writer  = this->h5Writer_;
+    auto& modelView = h5Writer.modelView();
+    auto& ions      = modelView.getIons();
+    auto minLvl     = this->h5Writer_.minLevel;
+    auto maxLvl     = this->h5Writer_.maxLevel;
+    // compute the momentum tensor for each population that requires it
+    // compute for all ions but that requires the computation of all pop
+    std::string tree{"/ions/"};
+    if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
+    {
+        auto computeMomentumTensor
+            = [&](GridLayout& layout, std::string patchID, std::size_t iLvel) {
+                  for (auto& pop : ions)
+                  {
+                      std::string tree{"/ions/pop/" + pop.name() + "/"};
+                      auto& pop_momentum_tensor = pop.momentumTensor();
+                      pop_momentum_tensor.zero();
+                      auto domainParts     = core::makeIndexRange(pop.domainParticles());
+                      auto patchGhostParts = core::makeIndexRange(pop.patchGhostParticles());
+
+                      // dumps occur after the last substep but before the next first substep
+                      // at this time, levelGhostPartsNew is emptied and not yet filled
+                      // and the former levelGhostPartsNew has been moved to levelGhostPartsOld
+                      auto levelGhostParts = core::makeIndexRange(pop.levelGhostParticlesOld());
+
+                      interpolator(domainParts, pop_momentum_tensor, layout, pop.mass());
+                      interpolator(patchGhostParts, pop_momentum_tensor, layout, pop.mass());
+                      interpolator(levelGhostParts, pop_momentum_tensor, layout, pop.mass());
+                  }
+                  ions.computeFullMomentumTensor();
+              };
+        modelView.visitHierarchy(computeMomentumTensor, minLvl, maxLvl);
+    }
+    else // if not computing total momentum tensor, user may want to compute it for some pop
+    {
+        for (auto& pop : ions)
+        {
+            std::string tree{"/ions/pop/" + pop.name() + "/"};
+
+            auto computePopMomentumTensor
+                = [&](GridLayout& layout, std::string patchID, std::size_t iLvel) {
+                      auto& pop_momentum_tensor = pop.momentumTensor();
+                      pop_momentum_tensor.zero();
+                      auto domainParts     = core::makeIndexRange(pop.domainParticles());
+                      auto patchGhostParts = core::makeIndexRange(pop.patchGhostParticles());
+
+                      // dumps occur after the last substep but before the next first substep
+                      // at this time, levelGhostPartsNew is emptied and not yet filled
+                      // and the former levelGhostPartsNew has been moved to levelGhostPartsOld
+                      auto levelGhostParts = core::makeIndexRange(pop.levelGhostParticlesOld());
+
+                      interpolator(domainParts, pop_momentum_tensor, layout, pop.mass());
+                      interpolator(patchGhostParts, pop_momentum_tensor, layout, pop.mass());
+                      interpolator(levelGhostParts, pop_momentum_tensor, layout, pop.mass());
+                  };
+            if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
+            {
+                modelView.visitHierarchy(computePopMomentumTensor, minLvl, maxLvl);
+            }
+        }
+    }
+}


The lambdas within the compute function could be refactored into separate functions for better readability and potential reuse.

coderabbitai · 2023-12-18T18:28:39Z

pyphare/pyphare/pharein/diagnostics.py

+
        if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
            error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(
                ElectromagDiagnostics.em_quantities


The error message in the ElectromagDiagnostics class could be more specific by stating that the provided quantity is not a valid electromag diagnostics quantity.

- error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join( + error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(

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.

Suggested change

if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:

error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(

ElectromagDiagnostics.em_quantities

if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:

error_msg = "Error: '{}' is not a valid electromag diagnostics quantity. Valid quantities are: " + ", ".join(

ElectromagDiagnostics.em_quantities

coderabbitai · 2023-12-18T18:28:39Z

pyphare/pyphare/pharein/diagnostics.py

+        if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:
            error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(
-                FluidDiagnostics.fluid_quantities
+                FluidDiagnostics_.fluid_quantities


The error handling in FluidDiagnostics_ for invalid quantities could be improved by providing a more informative error message.

- error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join( + error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(

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.

Suggested change

if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:

error_msg = "Error: '{}' not a valid fluid diagnostics : " + ", ".join(

FluidDiagnostics.fluid_quantities

FluidDiagnostics_.fluid_quantities

if kwargs["quantity"] not in FluidDiagnostics_.fluid_quantities:

error_msg = "Error: '{}' is not a valid fluid diagnostics quantity. Valid quantities are: " + ", ".join(

FluidDiagnostics_.fluid_quantities

coderabbitai · 2023-12-18T18:28:39Z

pyphare/pyphare/pharein/diagnostics.py

+        if kwargs["quantity"] not in ParticleDiagnostics.particle_quantities:
+            error_msg = "Error: '{}' not a valid particle diagnostics : " + ", ".join(
+                ParticleDiagnostics.particle_quantities
+            )
+            raise ValueError(error_msg.format(kwargs["quantity"]))


There is redundant code in the ParticleDiagnostics class where the quantity attribute is checked twice against particle_quantities.

- if kwargs["quantity"] not in ParticleDiagnostics.particle_quantities: - error_msg = "Error: '{}' not a valid particle diagnostics : " + ", ".join( - ParticleDiagnostics.particle_quantities - ) - raise ValueError(error_msg.format(kwargs["quantity"]))

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.

Suggested change

if kwargs["quantity"] not in ParticleDiagnostics.particle_quantities:

error_msg = "Error: '{}' not a valid particle diagnostics : " + ", ".join(

ParticleDiagnostics.particle_quantities

)

raise ValueError(error_msg.format(kwargs["quantity"]))

src/core/data/tensorfield/tensorfield.hpp

generalize vecfield to tensorfield add momentum tensor to ion and ion populations add pressure tensor to quantities proposed by Run add mass density diag and refac compute_hier_from get Patch convenience debug tool

coderabbitai

Review Status

Actionable comments generated: 2

Configuration used: .coderabbit.yaml

Commits

Files that changed from the base of the PR and between 2c44575 and 78d0536.

Files selected for processing (40)

pyphare/pyphare/core/gridlayout.py (3 hunks)
pyphare/pyphare/pharein/init.py (1 hunks)
pyphare/pyphare/pharein/diagnostics.py (10 hunks)
pyphare/pyphare/pharein/simulation.py (4 hunks)
pyphare/pyphare/pharesee/hierarchy.py (7 hunks)
pyphare/pyphare/pharesee/run.py (6 hunks)
src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 hunks)
src/amr/physical_models/hybrid_model.hpp (1 hunks)
src/core/CMakeLists.txt (1 hunks)
src/core/data/grid/gridlayoutdefs.hpp (1 hunks)
src/core/data/grid/gridlayoutimplyee.hpp (4 hunks)
src/core/data/ions/ion_population/ion_population.hpp (8 hunks)
src/core/data/ions/ions.hpp (13 hunks)
src/core/data/tensorfield/tensorfield.hpp (1 hunks)
src/core/data/vecfield/vecfield.hpp (2 hunks)
src/core/data/vecfield/vecfield_component.hpp (1 hunks)
src/core/hybrid/hybrid_quantities.hpp (2 hunks)
src/core/numerics/interpolator/interpolator.hpp (11 hunks)
src/diagnostic/detail/h5writer.hpp (14 hunks)
src/diagnostic/detail/types/electromag.hpp (3 hunks)
src/diagnostic/detail/types/fluid.hpp (10 hunks)
src/diagnostic/detail/types/particle.hpp (1 hunks)
src/diagnostic/diagnostics.hpp (1 hunks)
src/phare_core.hpp (3 hunks)
src/python3/patch_level.hpp (6 hunks)
src/restarts/detail/h5writer.hpp (1 hunks)
tests/amr/models/test_models.cpp (3 hunks)
tests/amr/resources_manager/test_resources_manager.cpp (3 hunks)
tests/core/data/electrons/test_electrons.cpp (6 hunks)
tests/core/data/gridlayout/test_laplacian.py (1 hunks)
tests/core/data/ion_population/test_ion_population.cpp (3 hunks)
tests/core/data/ions/test_ions.cpp (2 hunks)
tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
tests/diagnostic/init.py (2 hunks)
tests/diagnostic/test_diagnostics.hpp (2 hunks)
tests/diagnostic/test_diagnostics.ipp (1 hunks)
tests/simulator/test_diagnostic_timestamps.py (2 hunks)
tests/simulator/test_diagnostics.py (2 hunks)
tests/simulator/test_initialization.py (1 hunks)
tests/simulator/test_tagging.py (1 hunks)

Files skipped from review due to trivial changes (2)

src/core/CMakeLists.txt
src/core/data/grid/gridlayoutdefs.hpp

Additional comments: 1

tests/core/data/ion_population/test_ion_population.cpp (1)

60-61: The instantiation of IonPopulation with DummyVecField and DummyTensorField should be verified to ensure that the types and their methods are compatible with the expected behavior of IonPopulation.

coderabbitai · 2023-12-19T13:51:40Z

tests/core/data/ion_population/test_ion_population.cpp

@@ -25,11 +24,18 @@ struct DummyVecField
 {
    static constexpr std::size_t dimension = 1;
    using field_type                       = DummyField;
-    DummyVecField(std::string name, [[maybe_unused]] HybridQuantity::Vector v) { (void)name; }
+    DummyVecField(std::string name, HybridQuantity::Vector /*v*/) { (void)name; }


The constructor of DummyVecField is not using the parameter HybridQuantity::Vector. If this is intentional, consider removing the parameter or adding a comment explaining why it's commented out.

coderabbitai · 2023-12-19T13:51:40Z

tests/core/data/ion_population/test_ion_population.cpp

+struct DummyTensorField
+{
+    static constexpr std::size_t dimension = 1;
+    DummyTensorField(std::string name, HybridQuantity::Tensor /*v*/) { (void)name; }


The constructor of DummyTensorField is not using the parameter HybridQuantity::Tensor. Similar to the previous comment, consider removing the parameter or explaining its presence.

nicolasaunai added core diagnostics outputs refactoring labels Nov 7, 2023

nicolasaunai marked this pull request as draft November 7, 2023 10:31

coderabbitai bot reviewed Nov 7, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from eaf388d to fcfb6da Compare November 8, 2023 14:06

coderabbitai bot reviewed Nov 8, 2023

View reviewed changes

tests/core/data/ions/test_ions.cpp Outdated Show resolved Hide resolved

src/core/data/vecfield/vecfield.hpp Show resolved Hide resolved

src/core/data/vecfield/vecfield.hpp Show resolved Hide resolved

coderabbitai bot reviewed Nov 9, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from 5a003d5 to f23733f Compare November 9, 2023 17:09

coderabbitai bot reviewed Nov 9, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from f23733f to da875f4 Compare November 9, 2023 17:39

coderabbitai bot reviewed Nov 9, 2023

View reviewed changes

coderabbitai bot reviewed Nov 20, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch 2 times, most recently from 07d74f6 to c1939c4 Compare November 29, 2023 14:05

github-advanced-security bot found potential problems Nov 29, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from a848365 to 68a2297 Compare December 6, 2023 08:47

coderabbitai bot reviewed Dec 6, 2023

View reviewed changes

PhilipDeegan reviewed Dec 6, 2023

View reviewed changes

coderabbitai bot reviewed Dec 6, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from dd74cf6 to 1b25f15 Compare December 6, 2023 15:32

coderabbitai bot reviewed Dec 6, 2023

View reviewed changes

github-advanced-security bot found potential problems Dec 8, 2023

View reviewed changes

coderabbitai bot reviewed Dec 8, 2023

View reviewed changes

coderabbitai bot reviewed Dec 12, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch 2 times, most recently from 7783c4f to dea34ef Compare December 14, 2023 14:02

nicolasaunai requested a review from rochSmets December 14, 2023 14:04

github-advanced-security bot found potential problems Dec 14, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from dea34ef to 34bc8e0 Compare December 14, 2023 14:23

PhilipDeegan reviewed Dec 14, 2023

View reviewed changes

pyphare/pyphare/pharein/__init__.py Outdated Show resolved Hide resolved

PhilipDeegan reviewed Dec 14, 2023

View reviewed changes

src/amr/data/field/coarsening/field_coarsen_operator.hpp Show resolved Hide resolved

PhilipDeegan reviewed Dec 14, 2023

View reviewed changes

src/core/data/ions/ion_population/ion_population.hpp Outdated Show resolved Hide resolved

PhilipDeegan reviewed Dec 14, 2023

View reviewed changes

src/core/data/ions/ions.hpp Outdated Show resolved Hide resolved

PhilipDeegan reviewed Dec 14, 2023

View reviewed changes

tests/core/data/ion_population/test_ion_population.cpp Outdated Show resolved Hide resolved

PhilipDeegan reviewed Dec 14, 2023

View reviewed changes

src/diagnostic/detail/types/fluid.hpp Outdated Show resolved Hide resolved

PhilipDeegan reviewed Dec 17, 2023

View reviewed changes

coderabbitai bot reviewed Dec 18, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from 42cdad7 to 2391950 Compare December 18, 2023 16:01

coderabbitai bot reviewed Dec 18, 2023

View reviewed changes

nicolasaunai force-pushed the pressure branch from 2391950 to bec9a17 Compare December 18, 2023 18:10

coderabbitai bot reviewed Dec 18, 2023

View reviewed changes

PhilipDeegan reviewed Dec 18, 2023

View reviewed changes

src/core/data/tensorfield/tensorfield.hpp Outdated Show resolved Hide resolved

nicolasaunai and others added 3 commits December 19, 2023 14:27

add pressure tensor diagnostics

83280fb

generalize vecfield to tensorfield add momentum tensor to ion and ion populations add pressure tensor to quantities proposed by Run add mass density diag and refac compute_hier_from get Patch convenience debug tool

pr comments

94fac3e

switchless component access

78d0536

nicolasaunai force-pushed the pressure branch from bec9a17 to 78d0536 Compare December 19, 2023 13:35

coderabbitai bot reviewed Dec 19, 2023

View reviewed changes

PhilipDeegan approved these changes Dec 19, 2023

View reviewed changes

nicolasaunai merged commit 618c8f6 into PHAREHUB:master Dec 20, 2023
6 checks passed

		kwargs["diag_export_format"] = kwargs.get("diag_export_format", "hdf5")
		assert kwargs["diag_export_format"] in ["hdf5"] # only hdf5 supported for now

		@@ -69,7 +69,7 @@ void ElectromagDiagnosticWriter<H5Writer>::getDataSetInfo(DiagnosticProperties&
		std::string lvlPatchID = std::to_string(iLevel) + "_" + patchID;

		auto infoVF = [&](auto& vecF, std::string name, auto& attr) {

	auto infoVF = [&](auto& vecF, std::string name, auto& attr) {
	auto infoVF = [&lvlPatchID, &patchAttributes](auto& vecF, std::string name, auto& attr) {

		@@ -104,7 +104,7 @@
		auto vecFields = h5Writer.modelView().getElectromagFields();

		auto initVF = [&](auto& path, auto& attr, std::string key, auto null) {

	auto initVF = [&](auto& path, auto& attr, std::string key, auto null) {
	auto initVF = [&h5file, &h5Writer, this](auto& path, auto& attr, std::string key, auto null) {

		@@ -1249,6 +1255,41 @@ def compute_hier_from(h, compute):
		return PatchHierarchy(patch_levels, h.domain_box, refinement_ratio, time=t)

		@@ -81,203 +100,288 @@ def __init__(self, name, **kwargs):
		raise RuntimeError("A simulation must be created before adding diagnostics")


		return jx, jy, jz


		def _compute_current(patchdatas, **kwargs):


		Jx, Jy, Jz = _current2d(Bx, By, Bz, dx, dy)
		# return ({"name":"Jx", "data":Jx,"centering":"primal"},

		#include "restarts/restarts_props.hpp"

		#include "initializer/data_provider.hpp"


		if kwargs["quantity"] not in ElectromagDiagnostics.em_quantities:
		error_msg = "Error: '{}' not a valid electromag diagnostics : " + ", ".join(

		#ifndef PHARE_CORE_INCLUDE_HPP
		#define PHARE_CORE_INCLUDE_HPP

add Pressure diag #772

add Pressure diag #772

Conversation

nicolasaunai commented Nov 7, 2023 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot commented Nov 7, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 9, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolasaunai commented Nov 7, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 7, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

PhilipDeegan Dec 14, 2023 •

edited

Loading

PhilipDeegan Dec 18, 2023 •

edited

Loading