adding mass to bulk velocity #783
Conversation
|
Warning Rate Limit Exceeded@nicolasaunai has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 22 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes across the codebase involve the introduction of mass density calculations for ion populations, with adjustments to accommodate different masses within populations. A new Changes
TipsChat with CodeRabbit Bot (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nicolasaunai
force-pushed
the
bulkvelpop
branch
3 times, most recently
from
4906a2d to
9b4195e
Compare
nicolasaunai
force-pushed
the
bulkvelpop
branch
3 times, most recently
from
49e37b1 to
e262158
Compare
| if (!sameMasses_) | ||
| computeMassDensity(); | ||
| auto const& density = (sameMasses_) ? rho_ : massDensity_; | ||
|
|
||
| bulkVelocity_.zero(); | ||
| auto& vx = bulkVelocity_.getComponent(Component::X); |
There was a problem hiding this comment.
not sure why I can't do
auto& [vx, vy, vz] = bulkVelocity_();
nicolasaunai
force-pushed
the
bulkvelpop
branch
from
e262158 to
b105d7c
Compare
nicolasaunai
changed the title
nicolasaunai
force-pushed
the
bulkvelpop
branch
from
b105d7c to
1540ea6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- src/core/data/ions/ions.hpp (7 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (5 hunks)
Additional comments: 9
src/core/data/ions/ions.hpp (7)
28-34: The new type alias
Floatis introduced to standardize the floating-point type used across the codebase. This is a good practice for maintainability and consistency.49-54: The constructor now checks if all ion populations have the same mass and sets the
sameMasses_flag accordingly. Ensure that theallSameMass_()function is correctly implemented and that this logic does not introduce any side effects.106-127: The
computeMassDensity()function is added to calculate the mass density of ions. It is important to ensure that themassDensity_field is properly initialized before this function is called to avoid null pointer dereferences or undefined behavior.130-172: The
computeBulkVelocity()function has been updated to account for different population masses. The use ofsameMasses_to optimize the computation when all populations have the same mass is a good performance consideration. However, ensure that themassDensity_field is correctly computed before it is used in this function.180-187: The
isUsable()function has been updated to not checkmassDensity_as it is for internal use only. This is a reasonable decision, but it is important to ensure thatmassDensity_is always in a valid state when it is used internally.221-226: The
getFieldNamesAndQuantities()function now includes "massDensity" in the list of field names and quantities. This change aligns with the new functionality to compute mass density.233-242: The
setBuffer()function has been updated to handle the new "massDensity" buffer. It is crucial to ensure that the buffer names used throughout the codebase are consistent with these changes.tests/core/numerics/ion_updater/test_updater.cpp (2)
342-347: The copy constructor for
IonsBuffersshould ensure that thecopyDatamethod is called for all member fields. It seems thatcopyDatais not called forprotonFx,protonFy,protonFz,alphaFx,alphaFy,alphaFz,Vx,Vy, andVz. Ensure that these fields are also copied if necessary.372-378: In the
setBuffersmethod, the buffer forionMassDensityis set with the identifier "massDensity". This is correct and consistent with the proposed changes to the initialization ofionMassDensity. Ensure that all other parts of the code that reference this buffer are updated accordingly.
nicolasaunai
force-pushed
the
bulkvelpop
branch
from
1540ea6 to
da37b36
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- src/core/data/ions/ions.hpp (7 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (5 hunks)
Additional comments: 10
tests/core/numerics/ion_updater/test_updater.cpp (3)
259-266: The constructor of
IonsBuffersinitializes theionMassDensityfield. Ensure that the initialization of this field is consistent with the rest of the codebase and that theionMassDensityfield is being used as intended.307-314: The copy constructor of
IonsBuffersshould ensure that theionMassDensityfield is copied correctly. Verify that the copy constructor is being used appropriately and that theionMassDensityfield is handled correctly in all cases whereIonsBuffersis copied.372-378: The
setBuffersfunction ofIonsBufferssets the buffer for theionMassDensityfield. Ensure that the buffer is being set correctly and that theionMassDensityfield is being used as intended in the rest of the codebase.src/core/data/ions/ions.hpp (7)
28-34: The introduction of the
Floattype alias within thecorenamespace is a good practice for code clarity and potential changes to floating-point precision. Ensure that all floating-point types in the codebase are updated to use this alias for consistency.49-53: The constructor now checks if all ion populations have the same mass and sets the
sameMasses_flag accordingly. Ensure that this flag is correctly used throughout the codebase to optimize calculations when applicable.106-127: The new
computeMassDensity()function correctly computes the mass density of ion populations. Ensure that this function is called before any code that requires the mass density value.130-171: The
computeBulkVelocity()function has been updated to account for different population masses. Ensure that the mass density is computed before calling this function whensameMasses_is false. Also, verify that thebulkVelocity_is correctly used in subsequent calculations.180-187: The comment on lines 183-184 indicates that
massDensity_is for internal use only and is not checked byisUsable(). Ensure that this does not lead to any unintended side effects whenmassDensity_is required elsewhere in the code.221-225: The
getFieldNamesAndQuantities()function now includes "massDensity" in the returned properties. Verify that all consumers of this function can handle the new "massDensity" field.233-242: The
setBuffer()function has been updated to handle the "massDensity" buffer. Ensure that the buffer is correctly initialized and used wherever necessary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (9 hunks)
- src/amr/messengers/hybrid_messenger_info.hpp (1 hunks)
- src/amr/physical_models/hybrid_model.hpp (1 hunks)
- src/core/data/ions/ions.hpp (7 hunks)
- tests/simulator/test_restarts.py (3 hunks)
Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_messenger_info.hpp
Additional comments: 17
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (7)
121-126: The registration of resources such as
Jold_,NiOldUser_,massDensityOldUser_, andViOld_is done correctly according to the summary. However, it's important to ensure that these resources are properly managed elsewhere in the codebase, especially during deallocation to prevent memory leaks.142-147: The allocation of resources for
Jold_,NiOldUser_,massDensityOldUser_, andViOld_is consistent with the registration. It is crucial to verify that theallocateTimeparameter is being used correctly and that the allocated resources are being freed appropriately to avoid memory leaks.188-194: The registration of
massDensityGhostsRefiners_is consistent with the summary, which indicates the integration of mass density calculations into the ghost node refinement framework. Ensure that the registration ofmassDensityGhostsRefiners_is accompanied by the necessary logic to handle the newmassDensityOldUser_resource in other parts of the code.429-434: The filling of ghost nodes for
massDensityis implemented as described in the summary. It is important to ensure that theafterPushTimeparameter is being used correctly and that the filling process does not introduce inconsistencies in the simulation data.546-565: The
prepareStepmethod correctly copies the current state ofJ,Vi,Ni, andmassDensityto their respective "old" versions for use in time interpolation. It is essential to verify that thecurrentTimeparameter is being used correctly and that the copied data is consistent with the simulation's state at that time.681-690: The registration of time refiners for
massDensityis consistent with the summary. Ensure that thefieldRefineOp_andfieldTimeOp_operators are correctly implemented to handle the newmassDensityOldUser_resource.1002-1010: The initialization of
Jold_,ViOld_,NiOldUser_, andmassDensityOldUser_is consistent with the summary. It is important to verify that themassDensityOld_pointer is being managed correctly to prevent potential null pointer dereferences or memory leaks.src/amr/physical_models/hybrid_model.hpp (1)
- 149-155: The addition of
modelIonMassDensitytohybridInfois consistent with the summary provided. This change will require that any code usinghybridInfois updated to handle the newmodelIonMassDensityfield.src/core/data/ions/ions.hpp (7)
28-34: The addition of the
Floattype alias is a good practice for maintaining consistency in floating-point precision across the codebase.49-54: The constructor now checks if all populations have the same mass and sets the
sameMasses_flag accordingly. Ensure that this logic is consistent with the intended behavior of the simulation when populations are added or removed dynamically.60-95: The
density()andmassDensity()functions have been added to provide access to the density and mass density data. The error handling for unusable data is consistent and throws an exception, which is good practice.115-137: The
computeMassDensity()function correctly computes the mass density by summing the product of population density and mass. The use ofstd::transformwith a lambda for the accumulation is efficient and clear.142-182: The
computeBulkVelocity()function has been updated to account for varying population masses. The conditional logic based on thesameMasses_flag is clear and should work as intended. However, ensure that thecomputeMassDensity()function is called at the appropriate times if populations are added or removed dynamically.190-197: The
isUsable()function has been updated to not checkmassDensity_as it is for internal use only. This is a reasonable decision ifmassDensity_is indeed not meant to be accessed externally.232-253: The
getFieldNamesAndQuantities()andsetBuffer()functions have been updated to handle the newmassDensityfield. The error handling for invalid buffer names is consistent with the rest of the codebase.tests/simulator/test_restarts.py (2)
23-25: The comment on line 23 suggests that the
dimslist might be updated in the future to include more dimensions. Ensure that thepermutefunction and any related code are prepared to handle multiple dimensions when this change occurs.188-197: The
permutefunction is called withexpected_num_levels=1andexpected_num_levels=2, but the comment in the change summary indicates thatexpected_num_levelswas updated from 3 to 1 and from 2 to 1. Verify that the test cases are correctly reflecting the intended levels of refinement for the tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (9 hunks)
Additional comments: 9
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (9)
121-126: The registration of
massDensityOldUser_is correct and consistent with the summary.142-147: The allocation of
massDensityOldUser_is correct and consistent with the summary.188-194: The registration of
massDensityGhostsRefiners_is correct and consistent with the summary.429-435: The filling of
massDensityGhostsRefiners_infillIonMomentGhostsis correct and consistent with the summary.546-565: The
prepareStepmethod correctly sets the time and copies data formassDensityOldUser_, which is consistent with the summary.641-646: The
postSynchronizemethod correctly fills themassDensityGhostsRefiners_, which is consistent with the summary.681-690: The registration of
massDensityGhostsRefiners_inregisterGhostComms_is correct and consistent with the summary.1055-1061: The
GhostRefinerPoolformassDensityGhostsRefiners_is correctly declared and initialized, which is consistent with the summary.1002-1011: The declaration and initialization of
massDensityOldUser_are correct and consistent with the summary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (9 hunks)
- src/core/data/ions/ions.hpp (7 hunks)
- tests/simulator/test_restarts.py (9 hunks)
Additional comments: 21
src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (7)
121-126: The registration of resources such as
Jold_,NiOldUser_,massDensityOldUser_, andViOld_is done correctly in the constructor. However, it is important to ensure that these resources are also properly unregistered when they are no longer needed to avoid memory leaks.142-147: The allocation of resources within the
allocatemethod is correct. It is important to ensure that the allocated resources are deallocated appropriately when they are no longer in use.188-194: The registration of the
massDensityGhostsRefiners_is done correctly. It is important to verify that the corresponding de-registration logic is implemented when the level is unregistered or the object is destroyed.431-433: The commented-out line
// massDensityGhostsRefiners_.fill(level.getLevelNumber(), afterPushTime);suggests that there was an intention to fill the mass density ghosts, but it is currently not being done. If this is intentional, the commented code should be removed to avoid confusion. If it is an oversight, the code should be uncommented and verified to work as expected.546-565: The
prepareStepmethod correctly copies the data from the current state to the old state buffers (Jold_,NiOldUser_,massDensityOldUser_,ViOld_). Ensure that the copied data is used appropriately in subsequent calculations and that the buffers are updated at the correct times to maintain consistency.681-690: The registration of the
massDensityGhostsRefiners_andvelGhostsRefiners_is done correctly. It is important to verify that the corresponding de-registration logic is implemented when the level is unregistered or the object is destroyed.1002-1010: The fields
NiOld_andmassDensityOld_are raw pointers that are being wrapped inFieldUserobjects (NiOldUser_,massDensityOldUser_). It is crucial to ensure that the memory management for these pointers is handled correctly to prevent memory leaks or dangling pointers.src/core/data/ions/ions.hpp (6)
28-34: The introduction of the
Floattype alias is a good practice for maintaining consistency in floating-point precision across the codebase.49-53: The constructor now initializes the
sameMasses_flag by callingallSameMass_(). Ensure that this flag is correctly updated if populations are dynamically created or modified during the simulation, as indicated in the comment on lines 304-307.59-94: The
density()andmassDensity()functions have been updated to throw exceptions if the data is not usable. This is a good practice for error handling, but ensure that these exceptions are caught and handled properly wherever these functions are called.114-135: The
computeMassDensity()function correctly computes the mass density by summing the product of population density and mass. Ensure that the mass density field is initialized before this function is called.189-196: The
isUsable()function does not checkmassDensity_because it is for internal use only. Ensure thatmassDensity_is always in a valid state when used internally, as it is not checked for usability.230-253: The
getFieldNamesAndQuantities()andsetBuffer()functions have been updated to handle themassDensityfield. Ensure that themassDensityfield is correctly managed in the resource management framework.tests/simulator/test_restarts.py (8)
21-26: The
permutefunction has been updated to include a space after the comma in thedimsassignment. This is a minor style change and does not affect the functionality of the code.63-68: A new function
vxalphahas been added to calculate the velocity in the x-direction for alpha particles. This function is consistent with the existing velocity functions and follows the same pattern.77-84: The
vvvalphadictionary has been introduced to store velocity functions for alpha particles. This dictionary is correctly structured and follows the same pattern as thevvvdictionary for protons.95-104: The
alphadictionary within theMaxwellianFluidModelfunction has been modified to include thevvvalphadictionary. This change correctly integrates the new velocity functions for alpha particles into the model.171-187: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [171-203]
Assertions and error handling have been added within the
count_levels_and_patchesfunction. The error handling for the domain particles comparison is appropriate, and the use ofnp.testing.assert_equalfor array comparisons is correct.
211-220: The parameters passed to the
permutefunction in the test cases have been adjusted. The changes are consistent with the updatedpermutefunction and reflect the correct expected number of levels for the tests.249-252: The
restart_idxandrestart_timeare set correctly, and thetimestampslist is constructed properly to include the restart time and the final time step.261-266: The setup for the first simulation is correct, and the use of
dump_all_diagsto output diagnostics is appropriate. The simulator is run and then reset, which is the expected behavior.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- src/core/data/ions/ions.hpp (7 hunks)
Additional comments: 6
src/core/data/ions/ions.hpp (6)
28-34: The introduction of the
Floattype alias is a good practice for maintaining consistency in the representation of floating-point numbers across the codebase. Ensure that all floating-point types in theIonsclass and related classes are updated to use this alias.49-53: The constructor now initializes the
sameMasses_member by callingallSameMass_(). Ensure that this change does not introduce any side effects in the simulation where ion populations are dynamically added or removed after theIonsobject is constructed.59-94: The
density()andmassDensity()functions have been implemented to throw exceptions when the data is not usable. This is a good practice for error handling, but ensure that these exceptions are caught and handled properly wherever these functions are called.114-135: The
computeMassDensity()function correctly computes the mass density by summing the product of population density and mass. Ensure that the mass of each population is correctly initialized before this function is called to avoid incorrect calculations.189-196: The
isUsable()function does not check themassDensity_field because it is for internal use only. This is acceptable, but ensure that themassDensity_field is always in a valid state when used internally.231-253: The
getFieldNamesAndQuantities()andsetBuffer()functions have been updated to handle themassDensitybuffer. Ensure that all buffer names used throughout the codebase are consistent with these changes.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1 hunks)
- src/core/data/ions/ions.hpp (7 hunks)
Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
Additional comments: 6
src/core/data/ions/ions.hpp (6)
28-34: The introduction of the type alias
Floatis a good practice for maintainability and readability, especially if there's a possibility of changing the underlying type in the future. However, ensure that this alias is consistently used throughout the codebase where applicable.49-53: The constructor now initializes the
sameMasses_flag by calling theallSameMass_()function. Ensure that this flag is correctly updated if populations are dynamically created or modified during the simulation, as mentioned in the comment on lines 287-288.59-75: The
density()function now throws a runtime error if the density data is not usable. This is a good practice for error handling, but ensure that there are appropriate try-catch blocks where this function is called to handle the exception properly.96-117: The
computeMassDensity()function has been added to compute the mass density of ion populations. It uses a lambda function to multiply the population density by the population mass. This is a correct and efficient way to perform this operation. However, ensure thatmassDensity_is properly initialized before this function is called to prevent undefined behavior.171-178: The comment on lines 174-175 mentions that
massDensity_is not checked inisUsablebecause it is for internal use only. This is a reasonable decision, but it's important to ensure thatmassDensity_is always in a valid state when used internally to avoid any potential issues.212-235: The
getFieldNamesAndQuantities()andsetBuffer()functions have been updated to handle themassDensitybuffer. This is consistent with the changes made to include mass density in the simulation. Ensure that these functions are tested thoroughly to prevent any issues with buffer management.
nicolasaunai
force-pushed
the
bulkvelpop
branch
from
c63eae3 to
499763e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1 hunks)
- src/core/data/ions/ions.hpp (7 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (5 hunks)
- tests/simulator/test_restarts.py (9 hunks)
Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
Additional comments: 20
src/core/data/ions/ions.hpp (7)
28-34: The addition of the type alias
Floatand the related changes in theIonsclass are consistent with the summary provided. This change will help maintain consistency in the type used for floating-point operations throughout the class.49-53: The constructor now initializes the
sameMasses_member variable by calling theallSameMass_()function. This is a logical addition to optimize the bulk velocity computation when all ion populations have the same mass.59-75: The
density()function now throws a runtime error if the density data is not usable. This is a good practice for error handling, ensuring that the function fails fast and provides a clear error message if the preconditions are not met.83-84: The addition of the
massDensityName()function aligns with the summary, providing a consistent way to refer to the mass density field across the codebase.96-117: The
computeMassDensity()function has been added to compute the mass density of ion populations. This function is consistent with the summary and uses a lambda expression to apply the mass of each population to its density, summing the results to obtain the mass density.171-178: The comment on lines 174-175 provides clarity on why
massDensity_is not checked inisUsable(). This is a good practice for maintainability, as it explains the reasoning behind the implementation choices.213-235: The
getFieldNamesAndQuantities()andsetBuffer()functions have been updated to handle the newmassDensityfield. This change is consistent with the summary and ensures that the new field is integrated into the existing framework for resource management.tests/core/numerics/ion_updater/test_updater.cpp (5)
227-233: The type alias
ParticleInitializerFactoryis declared but not used in the provided code. If it is not used elsewhere, consider removing it to avoid unused code declarations.259-266: The
ionMassDensityfield is initialized with the sameHybridQuantity::Scalar::rhoasionDensity,protonDensity, andalphaDensity. This could be an error ifionMassDensityis supposed to represent a different physical quantity and should have a different initialization size. Please verify that the initialization size forionMassDensityis correct and that it should indeed be the same as for density fields.307-314: The copy constructor for
IonsBuffersinitializesionMassDensitywith the sameHybridQuantity::Scalar::rhoas the other density fields. As with the default constructor, verify that this is the intended behavior and thationMassDensityshould not have a different initialization size.342-347: The copyData method is used to copy the contents of the fields from the source
IonsBuffers. Ensure that the source buffers have been properly initialized before this copy operation to prevent copying uninitialized or invalid data.372-378: The
setBuffersmethod sets themassDensitybuffer for theionsobject. Verify that themassDensityfield is expected to be used in the same way as the other fields and that the corresponding buffer is properly allocated and initialized before being set.tests/simulator/test_restarts.py (8)
21-26: The
permutefunction has been updated with corrected indentation for thedimslist assignment. This change is purely stylistic and does not affect the functionality of the code.63-68: A new function
vxalphahas been added to calculate the velocity in the x-direction for alpha particles. This function is consistent with the summary of changes and is correctly implemented.74-84: A new dictionary
vvvalphahas been introduced to store velocity functions for alpha particles. This dictionary is consistent with the summary of changes and is correctly implemented.98-104: The
massvalue for alpha particles has been changed from4to4.0. This change ensures that the mass is explicitly a floating-point number, which is a good practice for clarity and to avoid potential type-related issues in calculations.171-187: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [171-203]
Assertions and error handling have been added to the
count_levels_and_patchesfunction. This is a good practice as it ensures the integrity of the data being compared in the diagnostics. The error messages provide additional context which can be helpful for debugging.
211-220: The
max_nbr_levelsvalue has been changed from3to1in thesimInputdictionary. This change is consistent with the summary of changes and is correctly implemented.249-252: The
timestampslist has been adjusted to use therestart_timevariable. This change is consistent with the summary of changes and is correctly implemented.264-264: A line registering a directory for cleanup has been commented out. This could potentially lead to directories not being cleaned up after tests, which may cause issues with disk space or test isolation. It's important to verify whether this change was intentional and if there are alternative cleanup mechanisms in place.
nicolasaunai
force-pushed
the
bulkvelpop
branch
from
499763e to
27feacc
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1 hunks)
- src/core/data/ions/ions.hpp (7 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (5 hunks)
- tests/simulator/test_restarts.py (8 hunks)
Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
Additional comments: 15
src/core/data/ions/ions.hpp (6)
31-31: The introduction of the
Floattype alias is a good practice for maintainability and flexibility in case the underlying type needs to change in the future.52-52: The
sameMasses_variable is correctly initialized in the constructor to optimize bulk velocity calculations when all ion populations have the same mass.102-117: The
computeMassDensitymethod is correctly implemented to calculate the mass density of ion populations, taking into account the mass of each population.123-163: The
computeBulkVelocitymethod is updated to handle different population masses, with an optimization for the case when all populations have the same mass.84-84: The
massDensityNamemethod is a good addition for consistency and maintainability, providing a centralized place to change the name of the mass density field if needed.221-235: The
setBuffermethod is correctly updated to handle the mass density buffer, with appropriate error handling for invalid buffer names.tests/core/numerics/ion_updater/test_updater.cpp (3)
227-233: The addition of the
ionMassDensityfield to theIonsBuffersstruct is correctly implemented with proper initialization and copying of data.372-378: The
setBuffersfunction in theIonsBuffersstruct has been correctly updated to include themassDensitybuffer.259-266: Verify that the allocation size for
ionMassDensityis intended to be the same as for number density fields (ionDensity,protonDensity,alphaDensity), as they all useHybridQuantity::Scalar::rho.tests/simulator/test_restarts.py (6)
21-26: The
permutefunction now includesexpected_num_levelsto parameterize test cases with the expected number of levels after simulation restarts.63-64: The
vxalphafunction has been added to calculate the x-direction velocity for alpha particles.77-84: The
vvvalphadictionary has been introduced to store velocity functions for alpha particles, which is a new feature to handle different particle types.98-98: The
massvalue for alpha particles has been updated from an integer to a float for consistency in numerical representation.211-220: The test case parameters
max_nbr_levelsandexpected_num_levelshave been adjusted to reflect changes in the simulation's expected behavior after restarts.249-251: The
restart_timeandtimestampsare set up for testing the simulation's restart functionality, ensuring that the simulation can continue from a specific point in time.
| using ParticleInitializerFactory = typename PHARETypes::ParticleInitializerFactory; | ||
|
|
||
| Field ionDensity; | ||
| Field ionMassDensity; | ||
| Field protonDensity; | ||
| Field alphaDensity; | ||
| Field protonFx; |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [27-27]
There is a typo in the ElectromagBuffers constructor: the Ey and Ez fields are incorrectly initialized with the string "EM_E_x". They should be initialized with "EM_E_y" and "EM_E_z" respectively.
- , Ey{"EM_E_x", HybridQuantity::Scalar::Ey, layout.allocSize(HybridQuantity::Scalar::Ey)}
- , Ez{"EM_E_x", HybridQuantity::Scalar::Ez, layout.allocSize(HybridQuantity::Scalar::Ez)}
+ , Ey{"EM_E_y", HybridQuantity::Scalar::Ey, layout.allocSize(HybridQuantity::Scalar::Ey)}
+ , Ez{"EM_E_z", HybridQuantity::Scalar::Ez, layout.allocSize(HybridQuantity::Scalar::Ez)}
nicolasaunai
force-pushed
the
bulkvelpop
branch
from
27feacc to
3abf8c5
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1 hunks)
- src/core/data/ions/ions.hpp (7 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (5 hunks)
- tests/simulator/test_restarts.py (7 hunks)
Files skipped from review due to trivial changes (1)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp
Additional comments: 13
src/core/data/ions/ions.hpp (4)
31-31: The introduction of the
Floattype alias enhances type consistency and readability.84-84: The
massDensityNamefunction correctly provides the name for the mass density field.102-117: The
computeMassDensityfunction is correctly implemented to zero themassDensity_field and compute the mass density.221-235: The
setBufferfunction is correctly implemented to handle the assignment of therho_andmassDensity_fields based on the buffer name.tests/core/numerics/ion_updater/test_updater.cpp (3)
227-233: The addition of the
ionMassDensityfield to theIonsBuffersstruct and its initialization in the constructor align with the summary of changes.307-314: The update to the
IonsBufferscopy constructor to include theionMassDensityfield is consistent with the summary.372-378: The modification to the
setBuffersfunction to handle theionMassDensitybuffer is in line with the summary.tests/simulator/test_restarts.py (6)
21-26: Verify that the test is intended to run only in one dimension as the
dimslist is hardcoded to[1].63-64: The addition of
vxalphafunction for alpha particles' x-velocity calculation looks good.77-83: The introduction of
vvvalphadictionary to store alpha particles' velocity functions is correctly implemented.97-103: The update to the
alphadictionary in theMaxwellianFluidModelto includevvvalphais correctly done.171-187: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [174-203]
The assertions and error handling within the
check_diagsmethod, which likely includes the functionality of the mentionedcount_levels_and_patchesfunction, are correctly implemented.
- 248-251: The adjustment to the
timestampslist in thetest_restartsfunction to use therestart_timevariable is correct and improves code readability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (16)
- .github/workflows/cmake_ubuntu.yml (1 hunks)
- pyphare/pyphare/pharein/init.py (2 hunks)
- res/cmake/def.cmake (2 hunks)
- src/core/data/ions/ions.hpp (7 hunks)
- src/core/data/particles/particle_packer.hpp (1 hunks)
- src/core/utilities/types.hpp (1 hunks)
- src/python3/CMakeLists.txt (3 hunks)
- tests/amr/messengers/test_messengers.cpp (1 hunks)
- tests/amr/models/test_models.cpp (1 hunks)
- tests/amr/resources_manager/test_resources_manager.cpp (1 hunks)
- tests/amr/tagging/test_tagging.cpp (1 hunks)
- tests/core/data/electrons/test_electrons.cpp (1 hunks)
- tests/core/data/ions/test_ions.cpp (1 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (6 hunks)
- tests/diagnostic/test_diagnostics.hpp (1 hunks)
- tests/initializer/test_initializer.cpp (1 hunks)
Files skipped from review due to trivial changes (4)
- tests/amr/tagging/test_tagging.cpp
- tests/core/data/electrons/test_electrons.cpp
- tests/core/data/ions/test_ions.cpp
- tests/diagnostic/test_diagnostics.hpp
Additional comments: 27
.github/workflows/cmake_ubuntu.yml (1)
- 83-83: The addition of
-DdevMode=ONenables development mode. Verify that this mode does not introduce any side effects that could affect the production build or the behavior of the software in a production environment.pyphare/pyphare/pharein/__init__.py (2)
102-106: The changes to
add_size_tfunction correctly ensure that only non-negative values are accepted, which aligns with the expected typesize_t. Good error handling with theRuntimeError.198-200: The check for negative population numbers before calling
add_size_tis a good practice to ensure data integrity.res/cmake/def.cmake (2)
11-11: The
PHARE_WERROR_FLAGSvariable is being set with the contents ofPHARE_FLAGS. Verify if this is the intended behavior, as it could unintentionally include flags that are not related to warnings or errors.38-38: The addition of the
-Wuninitializedflag to the_Werrvariable is consistent with the summary and is a good practice to catch potential issues with uninitialized variables.src/core/data/ions/ions.hpp (5)
28-34: The introduction of the
Floattype alias is a good practice for maintaining type consistency across the codebase.40-50: The constructor properly initializes the
sameMasses_member variable, which is a good practice for setting the initial state of the object.56-72: The error handling in the
density()functions is appropriate, throwing exceptions when the data is not usable.99-115: The use of a lambda in the
computeMassDensity()function for inline operations is efficient and maintains readability.266-270: The use of a custom tolerance in the
allSameMass_()function for floating-point comparison is a good practice to handle numerical precision issues.src/core/data/particles/particle_packer.hpp (1)
- 29-33: The change to use uniform initialization syntax in the
emptyfunction is a good practice as it ensures value-initialization of theparticleobject, which can prevent potential issues with uninitialized data.src/core/utilities/types.hpp (2)
329-346: The addition of the
Fntemplate parameter to theall,any, andnonefunctions is a good enhancement for flexibility, allowing custom predicate functions to be used.348-355: The addition of overloads for
float_equalsfor bothfloatanddoubletypes with default precision thresholds is a good enhancement for floating-point comparisons.src/python3/CMakeLists.txt (3)
4-10: The changes to the
cpptarget's compile options to use${PHARE_WERROR_FLAGS}and the setting of the library output directory are consistent and appear correct.15-21: The
cpp_dbgtarget has been correctly configured with additional preprocessor definitions for debugging purposes. Ensure that these new definitions are well-documented and that their impact is understood across the codebase.26-32: The
cpp_etctarget's changes mirror those of thecpptarget, using${PHARE_WERROR_FLAGS}and setting the library output directory. This maintains consistency across different build targets.tests/amr/messengers/test_messengers.cpp (1)
- 168-168: The change from
inttostd::size_tfordict["ions"]["nbrPopulations"]is consistent with the summary and improves type safety for representing sizes and counts.tests/amr/models/test_models.cpp (1)
- 54-54: The change from
inttostd::size_tfordict["ions"]["nbrPopulations"]is consistent with the summary and ensures type safety for size-related values.tests/amr/resources_manager/test_resources_manager.cpp (1)
- 50-50: The change from
inttostd::size_tfor "nbrPopulations" is consistent with the summary and improves type safety for size-related variables. Ensure that all parts of the code that interact with "nbrPopulations" are compatible with this change.tests/core/numerics/ion_updater/test_updater.cpp (6)
77-77: The change from
inttostd::size_tfornbrPopulationsis consistent with the summary and is a good practice for representing sizes.229-232: The addition of
ionMassDensitytoIonsBuffersstruct is consistent with the summary and is necessary for the new mass density calculations.374-375: The update to the
setBuffersfunction inIonsBuffersto set the buffer formassDensityaligns with the new field addition and is necessary for the mass density calculations.259-263: The constructor of
IonsBuffershas been updated to initialize the newionMassDensityfield, which is consistent with the addition of the new field and necessary for proper initialization.307-311: The copy constructor of
IonsBuffershas been updated to handle the newionMassDensityfield, ensuring that the mass density data is correctly copied when creating a new instance from an existing one.343-344: The
copyDatamethod is called for the newionMassDensityfield in the copy constructor ofIonsBuffers, which is necessary to ensure the data is correctly copied.tests/initializer/test_initializer.cpp (2)
71-77: The change from a reference to a non-reference variable for
pop0ParticleInitializeris not mentioned in the summary. Verify if this change is intentional and consider the implications on memory usage and performance, as a non-reference variable may involve a copy of the initializer object.71-77: The use of
InitFunction<1>forpop0densityaligns with the type consistency improvements mentioned in the summary. Ensure that all related code is updated to work with the templated type.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- src/initializer/python_data_provider.hpp (2 hunks)
- src/python3/cpp_simulator.hpp (1 hunks)
- src/python3/pybind_def.hpp (1 hunks)
Files skipped from review due to trivial changes (2)
- src/python3/cpp_simulator.hpp
- src/python3/pybind_def.hpp
Additional comments: 2
src/initializer/python_data_provider.hpp (2)
1-3: The removal of the comment block is a non-functional change and seems to be a cleanup task.
14-15: The change in the
#undefdirective fromHAVE_UNISTD_HPPtoHAVE_UNISTD_Haligns with standard header file naming conventions.
nicolasaunai
force-pushed
the
bulkvelpop
branch
from
b24c4d8 to
3a002f6
Compare
#782
Summary by CodeRabbit
New Features
Bug Fixes
ionMassDensityfield to ensure proper data handling.Refactor
Documentation