Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pressure Diag #769

Closed
wants to merge 4 commits into from
Closed

Conversation

nicolasaunai
Copy link
Member

@nicolasaunai nicolasaunai commented Nov 1, 2023

#752

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 for representing tensor fields.
    • Added momentum tensor components to the IonPopulation class.
    • Enhanced the Interpolator class to handle tensor fields.
    • Added computation of momentum tensors for diagnostic purposes in the FluidDiagnosticWriter class.
  • Refactor
    • Renamed the Writer class to H5Writer.
    • Replaced the VecField class with a TensorField alias.
  • Bug Fixes
    • Updated the componentMap function calls in various files.
  • Tests
    • Added tests for symmetric tensor fields and their integration into the particle and electromagnetic models.

@nicolasaunai
Copy link
Member Author

on top of #766

Copy link

coderabbitai bot commented Nov 1, 2023

Walkthrough

Walkthrough

The changes primarily involve the introduction of a new TensorField class and its integration into the existing codebase. This includes modifications to various classes and functions to handle the new tensor field, adjustments to data structures, and updates to tests. Additionally, there are changes to the Writer class, which is renamed to H5Writer, and updates to the Interpolator class to remove dependencies on VecField and GridLayout.

Changes

File(s) Summary
src/amr/data/field/coarsening/field_coarsen_operator.hpp Removed NO_DISCARD attribute and changed return type of a function.
src/core/CMakeLists.txt Added a new file tensorfield.hpp to the project.
src/core/data/grid/gridlayoutdefs.hpp Added new static variables for the components of the scalar M.
src/core/data/grid/gridlayoutimplyee.hpp Introduced new arrays for quantities related to Mxx, Mxy, Mxz, Myy, Myz, and Mzz.
src/core/data/ions/ion_population/ion_population.hpp Changes to the IonPopulation class template to handle the new tensor field.
src/core/data/ions/ions.hpp Added momentumTensor_ as a member variable and related functions.
src/core/data/tensorfield/tensorfield.hpp Introduced a C++ class template TensorField and an alias SymTensorField.
src/core/data/vecfield/vecfield.hpp Replaced the custom implementation with a using declaration that aliases VecField to TensorField.
src/core/data/vecfield/vecfield_component.hpp Changes to the Component enum and the Components struct to handle new components.
src/core/hybrid/hybrid_quantities.hpp Introduced new enum values for components of a hybrid quantity.
src/core/numerics/interpolator/interpolator.hpp Changes to the ParticleToMesh and Interpolator classes to remove dependencies and introduce a generic function object.
src/diagnostic/detail/h5writer.hpp Renamed the Writer class to H5Writer and its member functions accordingly.
src/diagnostic/detail/types/electromag.hpp Changes in the iteration over core::Components::componentMap.
src/diagnostic/detail/types/fluid.hpp Addition of a new member function compute to the FluidDiagnosticWriter class template.
src/diagnostic/diagnostics.hpp Changed the type of Writer_t from h5::Writer<ModelView_t> to h5::H5Writer<ModelView_t>.
src/phare_core.hpp Changes to the type aliases within the PHARE_Types struct.
src/python3/patch_level.hpp Replaced calls to the componentMap member function of the core::Components namespace.
src/restarts/detail/h5writer.hpp Changes in the included files.
tests/amr/models/test_models.cpp Addition of a new class SymTensorField1D and modifications to the types of IonsPop1D and Ions1D.
tests/amr/resources_manager/test_resources_manager.cpp Changes in the included files and updates to the type IonPopulation1D, Ions1D, and Electrons1D.
tests/core/data/electrons/test_electrons.cpp Addition of a new include statement and changes to the data structures and global variables.
tests/core/data/ion_population/test_ion_population.cpp Addition of a new struct DummyTensorField and modification of the AnIonPopulation struct.
tests/core/data/ions/test_ions.cpp Changes in the included files and updates to the type of "IonPopulation1D".
tests/core/numerics/ion_updater/test_updater.cpp Additions of momentum tensor fields and their respective buffers for protons and alpha particles.
tests/diagnostic/test_diagnostics.hpp Changes to the template struct Hi5Diagnostic and updates in the function checkVecField.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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 help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

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

@nicolasaunai nicolasaunai changed the title Add pressure drag Add pressure Diag Nov 1, 2023
@nicolasaunai nicolasaunai force-pushed the pressurereb branch 3 times, most recently from 0272592 to 2410004 Compare November 3, 2023 20:47
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 13

Configuration used: .coderabbit.yaml

Commits Files that changed from the base of the PR and between 32ae294 and 15730ea.
Files selected for processing (26)
  • src/amr/data/field/coarsening/field_coarsen_operator.hpp (2 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 (8 hunks)
  • src/diagnostic/detail/h5writer.hpp (14 hunks)
  • src/diagnostic/detail/types/electromag.hpp (2 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/test_diagnostics.hpp (2 hunks)
Files skipped from review due to trivial changes (2)
  • src/core/CMakeLists.txt
  • src/core/data/grid/gridlayoutdefs.hpp
Additional comments: 79
tests/amr/models/test_models.cpp (1)
  • 33-47: The introduction of SymTensorField1D and its integration into IonsPop1D and Ions1D is correctly done. The Electrons class now correctly takes Ions1D as a template parameter. These changes indicate the introduction of support for symmetric tensor fields and their integration into the particle and electromagnetic models.
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> class, or that all uses of Writer_t have been updated to match the new interface.
src/diagnostic/detail/types/electromag.hpp (2)
  • 69-75: Ensure that the change from core::Components::componentMap to core::Components::componentMap() is intentional and correct. If componentMap was a data member, this change could break the code. If it was a function, this change is correct.

  • 104-110: Similar to the previous comment, ensure that the change from core::Components::componentMap to core::Components::componentMap() is intentional and correct.

src/restarts/detail/h5writer.hpp (5)
  • 1-2: Ensure that the header guard is unique across the codebase.

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

Ensure that the Writer constructor and make_unique function are being called with the correct arguments in the codebase.

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

Ensure that the dump function is being called with the correct arguments in the codebase. Also, verify that the file paths and data being written are correct.

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

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

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

Ensure that the private members path_ and modelView_ are being used correctly in the codebase.

tests/diagnostic/test_diagnostics.hpp (2)
  • 36-41: The change from core::Components::componentMap to core::Components::componentMap() is correct if componentMap is a function. Ensure that this change is reflected throughout the codebase.

  • 45-51: The change from Writer<ModelView_t> to H5Writer<ModelView_t> is correct if H5Writer is the new class replacing Writer. Ensure that all instances of Writer have been replaced with H5Writer throughout the codebase.

src/phare_core.hpp (2)
  • 33-45: 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.

  • 49-58: The IonPopulation_t type alias has been updated to include SymTensorField_t as a template parameter. This change should be reflected in all instances where IonPopulation_t is used.

src/core/hybrid/hybrid_quantities.hpp (1)
  • 13-38: The addition of new enum values for momentum tensor components is a good way to handle the new tensor quantities. Ensure that these new enum values are used correctly throughout the codebase.
tests/core/data/ion_population/test_ion_population.cpp (2)
  • 29-37: The DummyTensorField struct is introduced here. Ensure that the isUsable and isSettable methods are correctly implemented according to the intended behavior of the tensor field.

  • 60-61: The IonPopulation struct is now instantiated with DummyTensorField. Make sure that all methods and properties of IonPopulation that interact with the tensor field are compatible with DummyTensorField.

src/core/numerics/interpolator/interpolator.hpp (4)
  • 276-295: The ParticleToMesh class has been updated to use a generic function object func for calculating the deposit. This change increases the flexibility of the class, allowing it to handle different types of deposits.

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

The ParticleToMesh class has been updated to use a generic function object func for calculating the deposit. This change increases the flexibility of the class, allowing it to handle different types of deposits.

  • 332-347: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [332-359]

The ParticleToMesh class has been updated to use a generic function object func for calculating the deposit. This change increases the flexibility of the class, allowing it to handle different types of deposits.

  • 493-510: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [472-510]

The Interpolator class has been updated to use the ParticleToMesh class for calculating the deposit for each component of the flux. This change simplifies the code and improves its maintainability.

src/diagnostic/detail/types/particle.hpp (1)
  • 4-11: The new includes seem to be in order and relevant to the changes described in the summary. Ensure that these new dependencies are properly managed in the build system.
tests/amr/resources_manager/test_resources_manager.cpp (2)
  • 1-9: The new includes for vecfield.hpp and tensorfield.hpp are correctly placed and necessary for the new TensorField class and its alias SymTensorField.

  • 29-38: The type definitions have been updated correctly to include the new SymTensorField1D as a template parameter for IonPopulation1D and Ions1D. The Electrons1D type now correctly uses Ions1D as a template parameter.

tests/core/data/ions/test_ions.cpp (1)
  • 34-43: The changes in the type aliases and class templates look good and are in line with the changes described in the pull request summary. Ensure that these changes are reflected throughout the codebase.
src/core/data/grid/gridlayoutimplyee.hpp (2)
  • 94-112: The new arrays for Mxx, Mxy, Mxz, Myy, Myz, and Mzz are initialized with data.primal for all components. Ensure that this is the intended behavior. Also, verify that the hybridQtyCentering array is updated correctly with these new quantities.

  • 311-334: The switch cases for Mxx, Mxy, Mxz, Myy, Myz, and Mzz have been added correctly. However, ensure that the indices gridData_.iMxx, gridData_.iMxy, gridData_.iMxz, gridData_.iMyy, gridData_.iMyz, and gridData_.iMzz are defined and initialized properly elsewhere in the code.

src/core/data/vecfield/vecfield.hpp (3)
  • 17-18: The VecField class has been replaced with a TensorField alias. Ensure that all instances of VecField in the codebase have been updated to reflect 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 TensorField instances. Ensure that the function is called correctly throughout the codebase.

  • 32-34: The VecFieldNames struct remains unchanged. It seems to be used for naming conventions in the VecField class. Since VecField is now an alias for TensorField, ensure that this struct is still relevant and used correctly.
src/core/data/tensorfield/tensorfield.hpp (1)
  • 1-290: The TensorField class template and its alias SymTensorField are well-structured and follow good practices. The class provides a clear interface for manipulating tensor fields and includes appropriate error handling. The use of constexpr for compile-time calculations and std::array for fixed-size arrays are good choices for performance and safety. The use of std::unordered_map for mapping component names to indices is a good choice for efficient lookups. The use of std::runtime_error for error handling is appropriate. The code is well-documented with comments explaining the purpose of each function. The use of delete for the copy constructor and copy assignment operator prevents copying of TensorField objects, which is a good practice for classes that manage resources. The use of default for the move constructor and move assignment operator enables efficient moving of TensorField objects. The use of NO_DISCARD attribute is a good practice to warn users when the return value of a function is discarded. The use of std::forward_as_tuple and std::make_index_sequence in the components function is a good practice for creating tuples of references to the components. The use of std::begin and std::end in the begin and end functions is a good practice for providing range-based for loop support. The copyData function checks the usability of both the source and destination tensor fields before copying data, which is a good practice for error handling. The setBuffer function checks the validity of the buffer name before setting the buffer, which is a good practice for error handling. The isUsable and isSettable functions check the usability and setability of the tensor field, which is a good practice for error handling. The getComponent and getComponentName functions check the usability of the tensor field before returning the component or component name, which is a good practice for error handling. The zero function checks the usability of the tensor field before zeroing it, which is a good practice for error handling.
src/core/data/vecfield/vecfield_component.hpp (3)
  • 12-12: New components have been added to the Component enum. Ensure that these are handled correctly throughout the codebase.

  • 16-25: The componentMap function now returns different maps based on the rank. This is a good approach to handle different types of components.

  • 35-41: The check function has been updated to include checks for the new components. This is a necessary change given the addition of new components.

src/core/data/ions/ions.hpp (10)
  • 7-11: The new includes are necessary for the changes in this file. Ensure that they are not duplicated elsewhere in the code.

  • 28-34: The new tensorfield_type is correctly defined and used.

  • 40-46: The momentumTensor_ member is correctly initialized in the constructor.

  • 91-96: The momentumTensor() member functions are correctly defined.

  • 143-161: The computeFullMomentumTensor() function correctly computes the full momentum tensor by summing over all populations. Ensure that the momentumTensor() of each population is correctly computed before this function is called.

  • 170-179: The isUsable() function correctly checks the usability of the momentumTensor_.

  • 182-192: The isSettable() function correctly checks the settable status of the momentumTensor_.

  • 204-210: The MomentProperties type has been correctly changed from std::vector to std::array. Ensure that all uses of this type in the codebase have been updated accordingly.

  • 234-238: The getCompileTimeResourcesUserList() function correctly includes the momentumTensor_.

  • 259-264: The momentumTensor_ member is correctly declared and initialized.

src/diagnostic/detail/h5writer.hpp (10)
  • 23-48: The class H5Writer is introduced to replace the Writer class. Ensure that all instances of the Writer class have been replaced with H5Writer throughout the codebase.

  • 54-64: The constructor of H5Writer is updated. Ensure that the constructor is called correctly with the new parameters in all instances.

  • 82-86: The method getDiagnosticWriterForType is introduced. Ensure that this method is used correctly in the codebase.

  • 142-148: The method writeVecFieldAsDataset is introduced. Ensure that this method is used correctly in the codebase.

  • 162-168: The typeWriters_ member variable is introduced and initialized. Ensure that this variable is used correctly in the codebase.

  • 182-185: The copy and move constructors and assignment operators are deleted to prevent shallow copying. This is a good practice for classes that manage resources.

  • 210-215: The dump method is updated. Ensure that this method is called correctly with the new parameters in all instances.

  • 237-243: The dump_level method is introduced. Ensure that this method is used correctly in the codebase.

  • 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 is introduced. Ensure that this method is used correctly in the codebase.

  • 297-301: The writeDatasets_ method is introduced. Ensure that this method is used correctly in the codebase.
src/core/data/ions/ion_population/ion_population.hpp (8)
  • 21-24: The new template parameter TensorField has been added to the IonPopulation class. Ensure that all instances of this class have been updated accordingly.

  • 33-40: The constructor now initializes a new member momentumTensor_. Ensure that the initializer object passed to the constructor contains the necessary data for this member.

  • 54-63: The isUsable and isSettable methods now also check the usability and setability of momentumTensor_. This is a logical change that should not cause any issues if the momentumTensor_ is properly initialized.

  • 191-195: New getter methods for momentumTensor_ have been added. These methods should work as expected if momentumTensor_ is properly initialized.

  • 208-214: The type of MomentProperties has been changed from std::vector to std::array. This change should not cause any issues as long as the size of the array is correctly managed.

  • 226-231: The type of ParticleProperties has been changed from std::vector to std::array. This change should not cause any issues as long as the size of the array is correctly managed.

  • 260-264: The return type of getCompileTimeResourcesUserList has been changed to include momentumTensor_. Ensure that all calls to this method have been updated accordingly.

  • 283-289: The new member momentumTensor_ has been added. Ensure that this member is properly initialized and managed throughout the class.

src/python3/patch_level.hpp (6)
  • 79-85: The componentMap function is now being called from the core::Components namespace instead of being a member function. Ensure that this change is consistent across the codebase and that the new function provides the same functionality as the old one.

  • 97-103: The componentMap function is now being called from the core::Components namespace instead of being a member function. Ensure that this change is consistent across the codebase and that the new function provides the same functionality as the old one.

  • 162-168: The componentMap function is now being called from the core::Components namespace instead of being a member function. Ensure that this change is consistent across the codebase and that the new function provides the same functionality as the old one.

  • 180-186: The componentMap function is now being called from the core::Components namespace instead of being a member function. Ensure that this change is consistent across the codebase and that the new function provides the same functionality as the old one.

  • 199-205: The componentMap function is now being called from the core::Components namespace instead of being a member function. Ensure that this change is consistent across the codebase and that the new function provides the same functionality as the old one.

  • 218-224: The componentMap function is now being called from the core::Components namespace instead of being a member function. Ensure that this change is consistent across the codebase and that the new function provides the same functionality as the old one.

tests/core/numerics/ion_updater/test_updater.cpp (6)
  • 241-258: The addition of momentum tensor fields for the IonsBuffers struct is consistent with the pull request summary. Ensure that these fields are used appropriately in the rest of the codebase.

  • 298-334: The initialization of the momentum tensor fields in the constructor is correct. The use of layout.allocSize ensures that the fields are allocated with the correct size.

  • 381-418: The copy constructor correctly initializes the momentum tensor fields. It's good that the code is consistent with the original constructor.

  • 467-473: The setting of the momentum tensor fields for the Ions object is correct. This is consistent with the changes made to the IonsBuffers struct.

  • 482-487: The setting of the momentum tensor fields for the proton population is correct. This is consistent with the changes made to the IonsBuffers struct.

  • 496-501: The setting of the momentum tensor fields for the alpha population is correct. This is consistent with the changes made to the IonsBuffers struct.

tests/core/data/electrons/test_electrons.cpp (6)
  • 3-7: The new include statement for "core/data/tensorfield/tensorfield.hpp" is added. Ensure that the path is correct and the file exists.

  • 129-134: The new type aliases SymTensorFieldND and IonPopulationND are introduced. Make sure they are used correctly throughout the code.

  • 152-163: New fields for the components of the momentum tensor are declared. Ensure that they are initialized before use.

  • 191-214: The fields for the components of the momentum tensor are allocated sizes. Ensure that the sizes are correct and that there is enough memory available.

  • 235-241: The buffers for the components of the momentum tensor are set. Ensure that the buffers are correctly initialized and that the keys used for setting the buffers are correct.

  • 250-255: The buffers for the components of the protons' momentum tensor are set. Ensure that the buffers are correctly initialized and that the keys used for setting the buffers are correct.

@@ -9,7 +9,7 @@
#include "core/data/vecfield/vecfield.hpp"
#include "core/hybrid/hybrid_quantities.hpp"


#include "core/data/tensorfield//tensorfield.hpp"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a double slash in the include path. This should be corrected to a single slash.

- #include "core/data/tensorfield//tensorfield.hpp"
+ #include "core/data/tensorfield/tensorfield.hpp"

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
#include "core/data/tensorfield//tensorfield.hpp"
#include "core/data/tensorfield/tensorfield.hpp"

Comment on lines +42 to +43
template<std::size_t rank, typename = std::enable_if_t<rank == 1 or rank == 2, void>>
using TensorType = std::conditional_t<rank == 1, Vector, Tensor>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TensorType template alias is a good way to handle different ranks of tensors. However, it currently only supports ranks 1 and 2. If you plan to support other ranks in the future, consider making this more flexible.

Comment on lines +66 to +70
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}};
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The componentsQuantities function for Tensor quantities currently only supports the momentum tensor M. If you plan to support other tensor quantities in the future, consider making this more flexible.

Comment on lines 54 to +57
* this return 0, meaning that this operator
* have the most priority
*/
NO_DISCARD int getOperatorPriority() const override { return 0; }
int getOperatorPriority() const override { return 0; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NO_DISCARD attribute was removed from the getOperatorPriority function. This attribute is used to warn developers when the return value of a function is discarded. If the return value of this function is important and should not be ignored, consider adding the NO_DISCARD attribute back.

Comment on lines 68 to 74
* In our case, we allow a RF up to 10, so having 5 ghost width is sufficient
*
*/
NO_DISCARD SAMRAI::hier::IntVector
getStencilWidth(SAMRAI::tbox::Dimension const& dim) const override
SAMRAI::hier::IntVector getStencilWidth(SAMRAI::tbox::Dimension const& dim) const override
{
return SAMRAI::hier::IntVector{dim, 2};
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NO_DISCARD attribute was removed from the getStencilWidth function and the return type was changed. If the return value of this function is important and should not be ignored, consider adding the NO_DISCARD attribute back. Also, ensure that the change in return type does not affect the rest of the codebase.

Comment on lines 130 to +140
};

auto infoVF = [&](auto& vecF, std::string name, auto& attr) {
for (auto const& [id, type] : core::Components::componentMap)
for (auto const& [id, type] : core::Components::componentMap<1>())
infoDS(vecF.getComponent(type), name + "_" + id, attr);
};

auto infoTF = [&](auto& tensorF, std::string name, auto& attr) {
for (auto const& [id, type] : core::Components::componentMap<2>())
infoDS(tensorF.getComponent(type), name + "_" + id, attr);
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The infoTF function is not used anywhere in the code. If it's not needed, consider removing it.

Comment on lines +159 to +160
// if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
// infoTF(momentum_tensor_["ion"], "momentum_tensor", patchAttributes[lvlPatchID]["ion"]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented-out code should be removed if it's not needed. If it's a placeholder for future implementation, consider adding a TODO comment.

Comment on lines 206 to 224
std::string popId{"fluid_" + pop.name()};
std::string tree{"/ions/pop/" + pop.name() + "/"};
std::string popPath(path + "pop/" + pop.name() + "/");
if (checkActive(tree, "density"))
if (isActiveDiag(diagnostic, tree, "density"))
initDS(path, attr[popId], "density", null);
if (checkActive(tree, "flux"))
if (isActiveDiag(diagnostic, tree, "flux"))
initVF(path, attr[popId], "flux", null);
// if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
// initTF(path, attr[popId], "momentum_tensor", null);
}

std::string tree{"/ions/"};
if (checkActive(tree, "density"))
if (isActiveDiag(diagnostic, tree, "density"))
initDS(path, attr["ion"], "density", null);
if (checkActive(tree, "bulkVelocity"))
if (isActiveDiag(diagnostic, tree, "bulkVelocity"))
initVF(path, attr["ion"], "bulkVelocity", null);
// if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
// initTF(path, attr["ion"], "momentum_tensor", null);
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initPatch function contains commented-out code at lines 213-214 and 222-223. If it's not needed, consider removing it. If it's a placeholder for future implementation, consider adding a TODO comment.

Comment on lines +261 to +262
// if (isActiveDiag(diagnostic, tree, "momentum_tensor"))
// writeTF(path + "momentum_tensor", momentum_tensor_["ion"]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented-out code should be removed if it's not needed. If it's a placeholder for future implementation, consider adding a TODO comment.

Comment on lines +43 to 47
inline std::unordered_map<std::string, Component> const Components::vecComponentMap{
{"x", Component::X}, {"y", Component::Y}, {"z", Component::Z}};
inline std::unordered_map<std::string, Component> const Components::tensComponentMap{
{"x", Component::X}, {"y", Component::Y}, {"z", Component::Z}};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vecComponentMap and tensComponentMap have been defined but they both map to the same components (X, Y, Z). The tensComponentMap should map to the tensor components (XX, XY, XZ, YY, YZ, ZZ).

- inline std::unordered_map<std::string, Component> const Components::tensComponentMap{
-     {"x", Component::X}, {"y", Component::Y}, {"z", Component::Z}};
+ inline std::unordered_map<std::string, Component> const Components::tensComponentMap{
+     {"xx", Component::XX}, {"xy", Component::XY}, {"xz", Component::XZ},
+     {"yy", Component::YY}, {"yz", Component::YZ}, {"zz", Component::ZZ}};

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
inline std::unordered_map<std::string, Component> const Components::vecComponentMap{
{"x", Component::X}, {"y", Component::Y}, {"z", Component::Z}};
inline std::unordered_map<std::string, Component> const Components::tensComponentMap{
{"x", Component::X}, {"y", Component::Y}, {"z", Component::Z}};
inline std::unordered_map<std::string, Component> const Components::vecComponentMap{
{"x", Component::X}, {"y", Component::Y}, {"z", Component::Z}};
inline std::unordered_map<std::string, Component> const Components::tensComponentMap{
{"xx", Component::XX}, {"xy", Component::XY}, {"xz", Component::XZ},
{"yy", Component::YY}, {"yz", Component::YZ}, {"zz", Component::ZZ}};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant