Charge #891
Charge #891
Conversation
…or pop.chargeDensity
WalkthroughWalkthroughThe changes introduced in this pull request involve comprehensive renaming and restructuring of variables related to ion and proton densities and momentum tensors in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
tests/simulator/test_advance.py (1)
312-312: Remove extraneousfprefix from the string.The
fprefix is unnecessary since there are no placeholders in the string. Remove thefto avoid confusion.Apply this change:
-fig.savefig(f"pd1.png") +fig.savefig("pd1.png")Tools
Ruff
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (40)
- pyphare/pyphare/pharein/diagnostics.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- pyphare/pyphare/pharesee/plotting.py (1 hunks)
- pyphare/pyphare/pharesee/run/run.py (3 hunks)
- pyphare/pyphare_tests/test_pharesee/test_hierarchy.py (1 hunks)
- src/amr/level_initializer/hybrid_level_initializer.hpp (1 hunks)
- src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (2 hunks)
- src/amr/physical_models/hybrid_model.hpp (1 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (1 hunks)
- src/core/data/electrons/electrons.hpp (4 hunks)
- src/core/data/ions/ion_population/ion_population.hpp (5 hunks)
- src/core/data/ions/ions.hpp (9 hunks)
- src/core/numerics/interpolator/interpolator.hpp (3 hunks)
- src/core/numerics/ion_updater/ion_updater.hpp (4 hunks)
- src/core/numerics/moments/moments.hpp (2 hunks)
- src/diagnostic/detail/types/fluid.hpp (5 hunks)
- src/phare/phare_init.py (1 hunks)
- src/phare/phare_init_small.py (1 hunks)
- src/python3/patch_level.hpp (2 hunks)
- tests/core/data/electrons/test_electrons.cpp (8 hunks)
- tests/core/data/ion_population/test_ion_population_fixtures.hpp (1 hunks)
- tests/core/data/ions/test_ions.cpp (1 hunks)
- tests/core/numerics/interpolator/test_main.cpp (5 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (12 hunks)
- tests/diagnostic/init.py (1 hunks)
- tests/diagnostic/test_diagnostics.hpp (4 hunks)
- tests/diagnostic/test_diagnostics.ipp (1 hunks)
- tests/functional/alfven_wave/alfven_wave1d.py (1 hunks)
- tests/functional/dispersion/dispersion.py (1 hunks)
- tests/functional/harris/harris_2d_lb.py (1 hunks)
- tests/functional/shock/shock.py (1 hunks)
- tests/functional/td/td1d.py (1 hunks)
- tests/functional/tdtagged/td1dtagged.py (1 hunks)
- tests/functional/translation/translat1d.py (2 hunks)
- tests/simulator/refinement/test_2d_10_core.py (1 hunks)
- tests/simulator/refinement/test_2d_2_core.py (1 hunks)
- tests/simulator/test_advance.py (5 hunks)
- tests/simulator/test_initialization.py (3 hunks)
- tests/simulator/test_run.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/core/data/ion_population/test_ion_population_fixtures.hpp
Additional context used
Path-based instructions (13)
src/core/numerics/moments/moments.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/ions/ion_population/ion_population.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/tagging/default_hybrid_tagger_strategy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/physical_models/hybrid_model.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/level_initializer/hybrid_level_initializer.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/ions/ions.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/python3/patch_level.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/ion_updater/ion_updater.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/data/electrons/electrons.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.tests/diagnostic/test_diagnostics.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/diagnostic/detail/types/fluid.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/numerics/interpolator/interpolator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Ruff
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
370-370: Local variable
coloris assigned to but never usedRemove assignment to unused variable
color(F841)
tests/simulator/test_advance.py
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 370-370: Unused local variable
Variable color is not used.
Additional comments not posted (108)
src/core/numerics/moments/moments.hpp (2)
18-18: LGTM!The change in function name from
densitytoparticleDensityimproves clarity and is consistent with the changes made in theIonPopulationclass.
43-44: LGTM!The changes in the
depositParticlesfunction improve clarity and add the ability to deposit charge density alongside particle density. The interpolation function now takes bothparticleDensityandchargeDensityas arguments, which is consistent with the changes made in theIonPopulationclass.Also applies to: 50-50, 55-55, 60-60
tests/diagnostic/__init__.py (2)
31-36: LGTM!The addition of "charge_density" and "mass_density" to the list of quantities enhances the diagnostic capabilities by including additional physical properties relevant to the fluid dynamics being analyzed. This change allows for a more comprehensive analysis of the fluid's characteristics.
44-44: LGTM!The addition of "charge_density" to the list of quantities broadens the scope of diagnostics performed on the populations, allowing for a more comprehensive analysis of the fluid's characteristics. This change reflects an enhancement in the diagnostic capabilities of the code.
src/phare/phare_init.py (1)
104-104: LGTM! The change aligns with the PR objectives.The modification of the
quantityparameter from "density" to "charge_density" in theFluidDiagnosticsfunction call is consistent with the overall goal of shifting the focus from general density measurements to charge density measurements, as outlined in the PR objectives and AI summary.This change will ensure that the appropriate diagnostic data is processed and analyzed, reflecting the emphasis on charge density.
src/phare/phare_init_small.py (1)
107-107: Verify the impact of changing from density to charge density.The change in the quantity being processed by the
FluidDiagnosticsfunction, from density to charge density, is consistent with the overall shift in focus indicated in the PR summary. This change looks good.However, it's important to ensure that the rest of the codebase is updated to handle this change. Please verify that any code that consumes the output of
FluidDiagnosticsis expecting charge density instead of density.You can use the following script to search for potential impacted areas in the codebase:
tests/functional/td/td1d.py (1)
103-103: LGTM! The change in fluid diagnostic quantity is appropriate.The modification of the
quantityparameter from"density"to"charge_density"in theFluidDiagnosticscall is consistent with the overall shift in focus towards charge density, as indicated in the AI-generated summary.This change suggests that subsequent analyses will specifically interpret and utilize charge density data rather than general density data. Ensure that any downstream code or visualizations are updated to reflect this change in diagnostic quantity.
tests/diagnostic/test_diagnostics.ipp (1)
17-17: LGTM!The change from
"/ions/density"to"/ions/charge_density"aligns with the PR objective of introducing the "charge_density" quantity. It reflects a shift in focus from measuring the density of ions to measuring their charge density, which may have implications for how the simulation interprets and processes ion-related data. The remaining diagnostic entries are preserved, maintaining the overall structure and functionality of the diagnostic management system.tests/functional/dispersion/dispersion.py (1)
174-174: LGTM!The change from
["density", "bulkVelocity"]to["charge_density", "bulkVelocity"]aligns with the overall theme of the pull request, which focuses on the addition and modification of the "charge_density" quantity across various components. This change is localized to theprescribedModesfunction and is unlikely to introduce any functional issues or bugs.tests/simulator/refinement/test_2d_2_core.py (1)
104-104: Diagnostic quantity change: "density" → "charge_density"The change in the diagnostic quantity from "density" to "charge_density" is consistent with the overall objective of the PR to focus on charge density measurements. This modification has semantic implications for how fluid dynamics are analyzed in the simulation, shifting from a general density measurement to a more specific charge density measurement.
The change is localized, does not introduce any syntax or logical errors, and appears to be an intentional and meaningful modification.
tests/functional/shock/shock.py (1)
110-110: Diagnostic quantity updated to focus on charge density.The change in diagnostic quantity from
"density"to"charge_density"aligns with the overall objective of emphasizing charge density in the simulation. This modification in the test setup reflects that shift in focus.Please verify if any downstream code relies on the
"density"diagnostic and needs to be updated to maintain consistency with this change. You can use the following command to search for references to"density"in Python files:Verification successful
Change from "density" to "charge_density" is part of a project-wide refactoring.
The modification in
shock.pyis consistent with a larger refactoring effort across the project, as evidenced by recent commits and widespread changes in multiple files. This shift from "density" to "charge_density" appears to be a deliberate architectural decision, likely aimed at improving the precision or clarity of the simulation model.Key points:
- Commit 92069af explicitly changes "ions.density" to "ions.chargeDensity" across numerous files.
- Similar changes are observed in other test files and core components.
- This refactoring affects multiple areas of the project, including initialization, diagnostics, and test setups.
The change is approved and aligns with the project's current direction. No further action is required for this specific modification.
Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type py $'"density"'Length of output: 5504
Script:
#!/bin/bash # Search for "charge_density" in Python files echo "Searching for 'charge_density' in Python files:" rg --type py '"charge_density"' echo -e "\nChecking recent commits for relevant changes:" git log -n 5 --grep="density" --pretty=format:"%h - %s" --name-statusLength of output: 4229
tests/simulator/refinement/test_2d_10_core.py (1)
104-104: Approve the change in diagnostic quantity.The change from "density" to "charge_density" aligns with the overall objective of the PR to shift the focus to charge density measurements. This modification enhances the specificity of the fluid diagnostics being collected.
To ensure consistency, please verify that this change from "density" to "charge_density" is applied uniformly across all relevant parts of the codebase, including other diagnostic classes, data handling modules, and test files. Inconsistencies could lead to errors or misinterpretation of simulation results.
Run the following script to search for potential inconsistencies:
Verification successful
Approve the change: Consistent implementation of "charge_density" confirmed.
The modification from "density" to "charge_density" in the diagnostic context has been consistently applied across the codebase. This change aligns with the PR's objective of focusing on charge density measurements. The remaining instances of "density" are appropriately used in different contexts, such as function definitions or population settings.
No inconsistencies or errors related to this change were found in the codebase. The implementation appears to be uniform and correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of "charge_density" across Python files. # Test 1: Search for "density" usage in Python files. # Expect: Only occurrences in comments, strings, or unrelated contexts. rg --type py 'density' # Test 2: Search for "charge_density" usage in Python files. # Expect: Occurrences in relevant diagnostic and data handling contexts. rg --type py 'charge_density'Length of output: 20998
tests/functional/translation/translat1d.py (2)
90-90: LGTM!The change from
densitytocharge_densityaligns with the PR objective and is accurately reflected in the AI-generated summary. The modification is localized and does not introduce any logical or structural issues.
185-185: LGTM!Similar to the previous instance, the change from
densitytocharge_densityaligns with the PR objective and is accurately reflected in the AI-generated summary. The modification is localized and does not introduce any logical or structural issues.tests/functional/alfven_wave/alfven_wave1d.py (1)
99-99: Verify thatFluidDiagnosticssupports the"charge_density"quantity.The change from
"density"to"charge_density"aligns with the overall theme of the pull request. However, please ensure that theFluidDiagnosticsclass has been updated to handle the"charge_density"quantity to avoid any potential runtime errors.Run the following script to verify that
FluidDiagnosticssupports the"charge_density"quantity:Verification successful
Verification successful:
FluidDiagnosticssupports the"charge_density"quantity.The change from
"density"to"charge_density"in thealfven_wave1d.pyfile is valid. TheFluidDiagnostics_class in./pyphare/pyphare/pharein/diagnostics.pyincludes"charge_density"in itsfluid_quantitieslist.However, to ensure consistency across the codebase:
- Please manually review other usages of
FluidDiagnosticswith"charge_density"to confirm that all relevant parts of the code have been updated.- Check if any documentation or comments need to be updated to reflect this change from
"density"to"charge_density".Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `FluidDiagnostics` supports the `"charge_density"` quantity. # Test: Search for the `FluidDiagnostics` class definition and check if it handles the `"charge_density"` quantity. # Expect: The `FluidDiagnostics` class should have a case for handling the `"charge_density"` quantity. rg --type python -A 10 $'class FluidDiagnostics'Length of output: 80
Script:
#!/bin/bash # Description: Search for FluidDiagnostics class and charge_density usage in Python files # Find Python files and search for FluidDiagnostics class echo "Searching for FluidDiagnostics class:" fd '\.py$' --exec grep -Hn -A 10 'class FluidDiagnostics' {} + echo -e "\nSearching for charge_density usage in FluidDiagnostics:" fd '\.py$' --exec grep -Hn 'charge_density.*FluidDiagnostics' {} +Length of output: 10577
tests/core/data/ions/test_ions.cpp (1)
157-157: LGTM!The test case modification aligns with the overall objective of the pull request to focus on charge density. The change is straightforward and does not introduce any correctness issues.
src/core/data/ions/ion_population/ion_population.hpp (6)
37-38: LGTM!The introduction of
particleDensity_andchargeDensity_member variables enhances clarity by explicitly differentiating between the two types of density. The initialization usingname_is consistent with the existing code style.
53-54: LGTM!The
isUsable()method is correctly updated to check the usability of the new density variables, ensuring consistency with the changes made to the member variables.
59-60: LGTM!The
isSettable()method is correctly updated to check the settable status of the new density variables, ensuring consistency with the changes made to the member variables.
85-89: LGTM!The new accessor methods for
particleDensity_andchargeDensity_are well-implemented. Providing both const and non-const versions allows for flexibility in accessing the variables. The use of theNO_DISCARDattribute is a good practice to prevent accidental discards of the return value.
108-109: LGTM!The
getCompileTimeResourcesViewList()method is correctly updated to include the new density variables in the returned tuple, ensuring consistency with the changes made to the member variables.
133-134: LGTM!The new density variables
particleDensity_andchargeDensity_are correctly declared as private member variables of typefield_type. Encapsulating the data as private members and providing controlled access through the accessor methods is a good practice.src/amr/tagging/default_hybrid_tagger_strategy.hpp (1)
39-39: LGTM! The change aligns with the PR objectives.The modification from
density()tochargeDensity()is consistent with the overall theme of focusing on charge density, as indicated by the PR title and summary. This change may impact how the model interprets ion-related data and could potentially affect the accuracy and behavior of the tagging strategy employed by the hybrid model. Ensure that this change is thoroughly tested and validated to confirm that it produces the expected results.tests/functional/harris/harris_2d_lb.py (2)
136-136: LGTM!The change from "density" to "charge_density" aligns with the overall theme of the PR to focus on charge density specifically. The change is consistent with the AI-generated summary and there are no apparent issues with the logic or syntax.
140-140: LGTM!The change from "density" to "charge_density" for the "protons" population aligns with the overall theme of the PR to focus on charge density specifically. The change is consistent with the AI-generated summary and the previous change at line 136. There are no apparent issues with the logic or syntax.
tests/simulator/test_run.py (1)
149-149: LGTM!The addition of
"charge_density"to the list of quantities for fluid diagnostics is consistent with the PR objective and does not introduce any apparent issues.tests/functional/tdtagged/td1dtagged.py (1)
120-120: Verify consistent renaming of"density"to"charge_density"across the codebase.The change from
"density"to"charge_density"in theFluidDiagnosticscall is semantically correct and aligns with the shift in focus to charge density measurement.To ensure the correctness and consistency of this change, please verify that:
- All other occurrences of
"density"(in the context of fluid diagnostics) have been appropriately renamed to"charge_density"across the codebase.- Any code that depends on or processes the fluid diagnostic quantities has been updated to handle the
"charge_density"quantity correctly.You can use the following script to search for relevant occurrences of
"density"andFluidDiagnosticsacross Python files in the codebase:Review the search results to confirm that:
- There are no remaining occurrences of
"density"inFluidDiagnosticscalls (indicating complete renaming).- All
FluidDiagnosticscalls now use"charge_density"instead (indicating consistent usage of the new quantity name).If any inconsistencies are found, please update them to maintain coherence across the codebase.
Verification successful
Renaming from "density" to "charge_density" consistently applied
The change from
"density"to"charge_density"in theFluidDiagnosticscall has been consistently implemented across the codebase. After a thorough analysis of the search results, we can confirm that:
- All relevant occurrences of
FluidDiagnosticsnow use"charge_density"instead of"density"for the quantity parameter.- File names and references to diagnostic outputs have been updated to use
"charge_density"consistently.- The term
"density"is still used appropriately in other contexts, such as function names and variable names, where it doesn't refer specifically to charge density diagnostics.No inconsistencies or missed renamings were found in the diagnostic-related code. The change has been correctly implemented and consistently applied throughout the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "Searching for occurrences of 'density' in FluidDiagnostics calls..." rg --type py 'FluidDiagnostics\(.*"density"' echo "Searching for occurrences of 'charge_density' in FluidDiagnostics calls..." rg --type py 'FluidDiagnostics\(.*"charge_density"'Length of output: 416
Script:
#!/bin/bash echo "Searching for FluidDiagnostics calls..." rg --type py 'FluidDiagnostics\(' -A 5 echo "Searching for occurrences of 'density' in Python files..." rg --type py 'density' echo "Searching for occurrences of 'charge_density' in Python files..." rg --type py 'charge_density'Length of output: 31109
src/amr/physical_models/hybrid_model.hpp (1)
157-157: LGTM!The change from
densityName()tochargeDensityName()enhances clarity and aligns with the overall focus on charge density. This improves the accuracy of the ion density representation within the hybrid model.src/amr/level_initializer/hybrid_level_initializer.hpp (1)
101-101: LGTM!The change from
ions.computeDensity()toions.computeChargeDensity()improves the expressiveness and precision of the code. It aligns with the overall theme of the pull request and does not introduce any apparent issues.src/core/data/ions/ions.hpp (13)
44-45: LGTM!The changes to the constructor align with the PR objective of focusing on charge density. The initialization of
chargeDensity_is consistent with other density variables.
60-61: LGTM!The changes to the
massDensity()accessors improve clarity by using themassDensity_variable directly.
63-64: LGTM!The new
chargeDensity()accessors are consistent with the accessors for other variables and provide both const and non-const overloads.
69-69: LGTM!The new
chargeDensityName()function improves maintainability by providing a single place to define the name of the charge density variable.
75-90: LGTM!The renaming of
computeDensity()tocomputeChargeDensity()improves clarity. The function correctly sums the charge densities of the populations intochargeDensity_, which is consistent with the overall changes in the class.
102-102: LGTM!The change to use
pop.particleDensity()incomputeMassDensity()is consistent with the renaming of thedensity()function in theIonPopulationclass. The logic of summing the mass densities remains correct.
113-113: LGTM!Calling
computeMassDensity()instead ofcomputeDensity()incomputeBulkVelocity()is consistent with the renaming ofcomputeDensity()and correctly computes the mass density needed for the bulk velocity.
130-134: LGTM!Using
plusMassinstead ofpluswhen transforming the velocity components incomputeBulkVelocity()correctly accounts for the mass of each population, as the bulk velocity is a mass-weighted average.
138-143: LGTM!Dividing the velocity components by
massDensity_instead ofrho_incomputeBulkVelocity()is consistent with the overall changes in the class and correctly computes the bulk velocity.
175-176: LGTM!Checking the usability of both
chargeDensity_andmassDensity_inisUsable()is correct, as both variables are now part of the class and should be usable for the class to be considered usable.
189-190: LGTM!Checking the settability of both
chargeDensity_andmassDensity_inisSettable()is correct, as both variables are now part of the class and should be settable for the class to be considered settable.
214-215: LGTM!Including both
chargeDensity_andmassDensity_in the tuple returned bygetCompileTimeResourcesViewList()is correct, as both variables are now part of the class and should be accessible through the resources view list.
239-239: LGTM!The addition of the
chargeDensity_member variable of typefield_typeis consistent with the overall changes in the class and correctly represents a scalar field for the charge density.src/python3/patch_level.hpp (2)
40-40: LGTM!The change from
density()tochargeDensity()aligns with the PR objective and is implemented correctly.
62-63: LGTM!The change from
density()tochargeDensity()for each ion population aligns with the PR objective and is implemented correctly.pyphare/pyphare/pharesee/run/run.py (2)
30-30: Renaming enhances clarity and precision.The change from
"ions_density"to"ions_charge_density"aligns with the overall theme of the pull request and provides more specificity about the type of density being represented.
119-119: LGTM!The changes to the hierarchy file name in the
GetNiandGetPemethods are consistent with the renaming of the dictionary key at line 30. This ensures that the correct data is being accessed and processed, enhancing the overall consistency and maintainability of the codebase.Also applies to: 156-156
src/core/numerics/ion_updater/ion_updater.hpp (2)
110-110: LGTM!The change from
computeDensity()tocomputeChargeDensity()aligns with the PR objective of focusing on charge density calculations across the codebase.
161-161: LGTM!The change to pass
pop.particleDensity()andpop.chargeDensity()to theinterpolator_provides a more granular approach to handling different types of densities, which may enhance the fidelity of the simulation.src/core/data/electrons/electrons.hpp (4)
68-68: LGTM!The change from
ions_.density()toions_.chargeDensity()aligns with the PR objective and AI summary. It localizes the focus on charge density, which is appropriate in this context.
81-81: Looks good!This change maintains consistency with the previous modification and aligns with the overall PR objective.
115-115: The changes look good!Assigning
ions_.chargeDensity()toNeand using it to adjust the electron velocities based on the ion current densities is physically accurate. It properly accounts for the net charge distribution, which is the correct approach. The changes align with the PR objective and do not introduce any apparent issues.Also applies to: 129-131
214-214: Looks good to me!Assigning
ions_.chargeDensity()toNe_and using it to calculate the electron pressure is the correct approach. It maintains consistency with the other changes and aligns with the PR objective. The modification is localized and does not introduce any apparent issues.pyphare/pyphare/pharesee/plotting.py (1)
277-277: Change aligns with the pull request's focus on charge density. Verify the new data file.The modification of the file path to access
"ions_charge_density.h5"instead of"ions_density.h5"is consistent with the overall goal of incorporating charge density data.Please ensure that the
"ions_charge_density.h5"file exists at the specified path and contains the expected data structure to avoid runtime errors.tests/diagnostic/test_diagnostics.hpp (4)
125-125: LGTM!The changes are consistent with the overall transition from density to charge density. The variable names are updated appropriately.
Also applies to: 128-128
Line range hint
140-165:
Line range hint
168-208:
229-229: Looks good!The changes consistently add support for the new charge density attribute:
- The expected number of population attribute files is increased to account for charge density.
- The list of expected HDF5 file types now includes charge density at both the ions and population-specific levels.
- Population-specific checks now include charge density.
The changes are localized and maintain the overall function logic.
Also applies to: 239-240, 251-251
pyphare/pyphare/pharein/diagnostics.py (1)
225-225: LGTM!The addition of "charge_density" to the list of fluid quantities is a valid and useful extension of the
FluidDiagnostics_class. It enables more comprehensive diagnostics of fluid properties by incorporating charge density measurements. This change aligns well with the overall theme of introducing charge density diagnostics in this pull request.src/diagnostic/detail/types/fluid.hpp (5)
149-150: LGTM!The changes from "density" to "charge_density" for both ion populations and ions align with the overall shift in the diagnostic framework. The modifications are consistent and do not introduce any apparent issues.
Also applies to: 154-155
203-204: LGTM!The addition of conditions to retrieve dataset information for "charge_density" of both ion populations and ions aligns with the introduction of charge density as a new diagnostic metric. The changes are consistent with the overall modifications in the diagnostic framework and do not introduce any apparent issues.
Also applies to: 212-213
274-275: LGTM!The addition of conditions to initialize datasets for "charge_density" of both ion populations and ions aligns with the introduction of charge density as a new diagnostic metric. The changes are consistent with the overall modifications in the diagnostic framework and do not introduce any apparent issues.
Also applies to: 283-284
315-317: LGTM!The changes from writing "density" to "charge_density" for ion populations and the addition of the condition to write "charge_density" for ions align with the overall shift in the diagnostic framework. The modifications are consistent and do not introduce any apparent issues.
Also applies to: 325-326
355-355: LGTM!The addition of the condition to check and write attributes for "charge_density" of ion populations aligns with the introduction of charge density as a new diagnostic metric. The change is consistent with the overall modifications in the diagnostic framework and does not introduce any apparent issues.
pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1)
31-31: LGTM!The addition of the
"charge_density": "rho"entry to thefield_qtiesdictionary is a valid extension to accommodate charge density alongside other density-related quantities. This change enhances the clarity and usability of the code when dealing with charge density in the context of the application.tests/core/data/electrons/test_electrons.cpp (4)
129-130: LGTM!The changes to the struct template parameters are clear and consistent.
Line range hint
151-208: Great job with the variable renames!The new variable names for ion and proton densities and momentum tensors greatly improve code readability. The corresponding updates in the
_ionsfunction and constructor are consistent and maintain the correct functionality.
Line range hint
247-323: Update looks good!The constructor has been correctly updated to use the new
ionChargeDensityvariable for filling values. The change is consistent with the variable rename and maintains the existing functionality.
373-373: Looks good!The test function has been correctly updated to use
ions.chargeDensity()instead ofNibuffer. This change is consistent with the variable rename and maintains the test's purpose of comparing electron and ion densities.src/core/numerics/interpolator/interpolator.hpp (3)
472-474: LGTM!The addition of the
chargeDensityparameter and the corresponding call toparticleToMesh_usingpart.chargelook good. This allows the function to handle charge density alongside particle density.
493-495: LGTM!The new call to
particleToMesh_forchargeDensityusing a lambda that returnspart.chargeis consistent with the changes made to the function signature. This ensures that the charge density is calculated correctly using the charge property of each particle.
509-512: LGTM!The addition of the
chargeDensityparameter to the function signature and passing it to the call to(*this)maintain consistency with the changes made to the other overload ofoperator(). This ensures that the charge density is correctly passed and processed.tests/core/numerics/interpolator/test_main.cpp (4)
Line range hint
519-530: Looks good!The addition of the
rho_cgrid variable and its initialization in the constructor is a clean change. It follows the existing pattern for therhovariable.
637-637: Verify the usage ofrho_cin the interpolation logic.The
interpolatorfunction call has been updated to pass the newrho_cvariable. Please ensure that the interpolation logic correctly handles this additional parameter.Consider adding a test case to verify that
rho_cis being used as expected in the interpolation process and that the results are correct.
683-690: Looks good!The addition of the
rho_cgrid variable and its initialization in the constructor is a clean change. It follows the existing pattern for therhovariable.
704-704: Verify the usage ofrho_cin the interpolation logic.The
interpolatorfunction call has been updated to pass the newrho_cvariable. Please ensure that the interpolation logic correctly handles this additional parameter.Consider adding a test case to verify that
rho_cis being used as expected in the interpolation process and that the results are correct.pyphare/pyphare/pharesee/hierarchy/hierarchy.py (4)
356-357: LGTM!The import statement for the
Rectangleclass is valid and necessary for the code that follows.
368-375: LGTM!The initialization of the figure, axis, and arrays for representing the box colors and coordinates looks good. The increased figure size allows for a more spacious layout.
Tools
Ruff
370-370: Local variable
coloris assigned to but never usedRemove assignment to unused variable
color(F841)
GitHub Check: CodeQL
[notice] 370-370: Unused local variable
Variable color is not used.
378-385: LGTM!The code correctly maps the
facecolorvalues from each collection to the corresponding elements in theijarray based on the box coordinates. It also handles the case wherefacecoloris not specified by defaulting to NaN.
387-409: LGTM!The code effectively visualizes the boxes using
pcolormesh()based on theix,iy, andijarrays. Setting the x and y ticks aligns the plot with the level domain box coordinates. The addition of red rectangles with no fill nicely highlights the patch boundaries. The optional x and y limits throughkwargsprovide flexibility in controlling the plot range.tests/simulator/test_initialization.py (2)
Line range hint
27-33: **** The_densityfunction has no changes in this pull request. Skipping the review.
173-173: Changes look good!The changes consistently update the terminology from "density" to "charge_density" for fluid diagnostics and moments hierarchy. This aligns with the pull request's objective of enhancing the representation of fluid properties.
Please ensure that any code dependent on these quantities and filenames is updated accordingly to maintain compatibility.
Also applies to: 235-235
tests/simulator/test_advance.py (7)
50-50: Approve the addition of the seed value inmodel_init.The inclusion of a seed value in the
model_initparameter is a good practice to ensure reproducibility of the tests. It will help maintain consistency in the initialization of the simulation hierarchy across test runs.Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
155-155: Approve the renaming of diagnostic quantities and filenames.The change from "density" to "charge_density" for diagnostic quantities and filenames is a semantic improvement that better represents the nature of the data being analyzed. This renaming does not introduce any functional changes and enhances the clarity of the codebase.
Also applies to: 225-225, 234-234
292-292: Approve the adjustment of assertion tolerance and error visualization.The increase in the assertion tolerance from
5.5e-15to2.5e-14accommodates for potential larger discrepancies in the floating-point comparisons, making the tests more resilient to minor variations.The addition of error visualization code is a valuable improvement for debugging and understanding discrepancies in the test results. By plotting the 2D patches with distinct colors for the compared datasets and saving the figure, it becomes easier to identify and analyze any differences between the slices when an assertion fails.
These changes enhance the robustness and debugging capabilities of the testing framework.
Also applies to: 296-313
Line range hint
328-338: Skipping review of_test_overlaped_fields_are_equal.There are no significant changes to this function, so a detailed review is not necessary.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
340-382: Skipping review of_test_overlapped_particledatas_have_identical_particles.There are no significant changes to this function, so a detailed review is not necessary.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
384-403: Skipping review of_test_L0_particle_number_conservation.There are no significant changes to this function, so a detailed review is not necessary.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
405-539: Skipping review of_test_field_coarsening_via_subcycles.There are no significant changes to this function, so a detailed review is not necessary.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
tests/core/numerics/ion_updater/test_updater.cpp (6)
214-214: LGTM: The renaming and addition of member variables improves code clarity.The changes to distinguish between particle and charge densities for ions, protons, and alpha particles enhances the readability and maintainability of the
IonsBuffersstruct.Also applies to: 216-219
242-253: LGTM: The constructor correctly initializes the new density member variables.The initialization of
ionChargeDensity,protonParticleDensity,protonChargeDensity,alphaParticleDensity, andalphaChargeDensityin the constructor looks good.
279-290: LGTM: The copy constructor correctly initializes and copies data for the new density member variables.The initialization and data copying for
ionChargeDensity,protonParticleDensity,protonChargeDensity,alphaParticleDensity, andalphaChargeDensityin the copy constructor looks good.Also applies to: 313-318
328-331: LGTM: ThesetBuffersmethod correctly sets the new density buffers.The setting of
ionChargeDensity,ionMassDensity,protonParticleDensity,protonChargeDensity,alphaParticleDensity, andalphaChargeDensitybuffers in thesetBuffersmethod looks good.Also applies to: 337-339, 346-348
577-581: LGTM: The test fixture has been updated to use the new density variables.The
checkMomentsHaveEvolvedandcheckDensityIsAsPrescribedmethods in theIonUpdaterTeststruct have been correctly updated to use the newprotonParticleDensity,protonChargeDensity,alphaParticleDensity, andalphaChargeDensityvariables, consistent with the changes in theIonsBuffersstruct.Also applies to: 583-587, 616-617, 622-623, 668-670, 672-673
927-929: LGTM: The test correctly uses the newparticleDensityvariable.The usage of
pop.particleDensity()to access the density in thethatNoNaNsExistOnPhysicalNodesMomentstest is correct and consistent with the changes in theIonsBuffersstruct.src/amr/messengers/hybrid_hybrid_messenger_strategy.hpp (14)
Line range hint
81-88: LGTM!The constructor correctly initializes the member variables and registers the necessary resources with the resource manager.
Line range hint
97-103: LGTM!The
allocatefunction correctly allocates the messenger strategy's internal variables to the model's resource manager on the given patch at the specified time.
Line range hint
115-124: LGTM!The
registerQuantitiesfunction correctly creates SAMRAI algorithms for communications associated between pairs of variables by calling the appropriate registration functions with thehybridInfo.
Line range hint
134-169: LGTM!The
registerLevelfunction correctly notifies allRefinerPools andSynchronizerPools that the levellevelNumbernow exists. It also handles the special case of the root level by skipping the creation of schedules that use coarser level data.
Line range hint
179-220: LGTM!The
regridfunction correctly performs the regriding communications for Hybrid to Hybrid messengers. It regrids the necessaryRefinerPools, fills ghost cells for magnetic and electric fields, fixes magnetic field divergence, and handles the special case for regriding level 0.
Line range hint
243-269: LGTM!The
initLevelfunction correctly initializes hybrid data on the levellevelNumberat timeinitDataTimefrom hybrid coarser data. It fills the necessary field initializers, domain particles, patch ghost particles, and level ghost particles old. It also copies the level ghost old particles to pushable particles.
Line range hint
283-288: LGTM!The
fillElectricGhostsfunction correctly fills the electric field ghost cells by using the appropriate refiners for shared nodes and ghost nodes.
Line range hint
294-299: LGTM!The
fillCurrentGhostsfunction correctly fills the current density ghost cells by using the appropriate refiners for shared nodes and ghost nodes.
Line range hint
310-322: LGTM!The
fillIonGhostParticlesfunction correctly fills the interior ghost particle array from neighbor patches of the same level. It empties the array for all populations before filling the patch ghost particles using the appropriate refiner.
Line range hint
334-406: LGTM!The
fillIonPopMomentGhostsfunction correctly handles moment ghost nodes. It completes patch border node moments by depositing patch ghost particles and level border nodes by depositing level ghost [old,new] particles using linear time interpolation. The function calculates the interpolation coefficientalphaand applies it appropriately for levels other than the root level.
Line range hint
410-416: LGTM!The
fillIonMomentGhostsfunction correctly fills pure (patch and level) ghost nodes by applying a regular ghost schedule. It fills the total density usingrhoGhostsRefiners_and bulk velocity usingvelGhostsRefiners_, while noting that population densities and fluxes are not filled on any ghost nodes.
Line range hint
426-448: LGTM!The
firstStepfunction correctly handles getting level border ghost particles from the next coarser level. It checks for a valid time progression, fills level ghost particles new usinglvlGhostPartNewRefiners_, and updates the coarse time variables for levels other than the root level. The function appropriately does nothing for the root level.
Line range hint
459-494: LGTM!The
lastStepfunction correctly performs the necessary operations at the last step of a substepping cycle for hybrid-hybrid messages. For levels other than the root level, it moveslevelGhostParticlesNewparticles intolevelGhostParticlesOld, emptieslevelGhostParticlesNew, and copies the newCoarseToFineOldcontent tolevelGhostParticles. For the root level, it validates that the level ghost particle arrays are empty. The function handles the level-specific logic appropriately.
Line range hint
504-548: LGTM!The
prepareStepfunction correctly implements theHybridMessengerStrategy::prepareStepmethod for hybrid-Hybrid communications. It saves the t=n versions of J, Ni, and Vi into the messenger by copying the data from the current versions to their respective old versions and setting the appropriate time on each
| @@ -145,7 +145,7 @@ def vthz(x, y): | |||
| for quantity in ["E", "B"]: | |||
| ph.ElectromagDiagnostics(quantity=quantity, write_timestamps=timestamps) | |||
|
|
|||
| for quantity in ["density", "bulkVelocity"]: | |||
| for quantity in ["charge_density", "bulkVelocity"]: | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Review needed: "charge_density" usage impacts multiple files
The change from "density" to "charge_density" is part of a broader modification across the codebase. While the change in the reviewed file is correct, there are potential implications:
- Multiple files use both "density" and "charge_density" in various contexts, including FluidDiagnostics calls.
- The coexistence of both terms suggests a need for careful review to ensure consistency.
Action items:
- Review all instances of "density" usage to determine if they should be updated to "charge_density".
- Update relevant documentation to reflect the change in terminology.
- Ensure that the change is consistently applied in all FluidDiagnostics calls and other relevant contexts.
This change may have a broader impact than initially anticipated. A thorough review of the codebase is recommended to maintain consistency and prevent potential issues.
Analysis chain
Looks good! Verify if downstream code needs to be updated.
The change from "density" to "charge_density" is consistent with the PR objective.
Please ensure that any downstream code relying on the "density" diagnostic is updated to use "charge_density" instead.
Run the following script to check if the "density" diagnostic is used elsewhere in the codebase:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the "density" diagnostic is used elsewhere in the codebase.
# If so, those instances may need to be updated to "charge_density".
rg --type python $'FluidDiagnostics\(.*quantity="density"'
Length of output: 90
Script:
#!/bin/bash
# Check available file types
echo "Available file types:"
rg --type-list
# Search for "density" and "charge_density" in Python files
echo -e "\nSearching for 'density' in Python files:"
rg --glob '*.py' 'FluidDiagnostics\(.*quantity="density"'
echo -e "\nSearching for 'charge_density' in Python files:"
rg --glob '*.py' 'FluidDiagnostics\(.*quantity="charge_density"'
Length of output: 5221
Script:
#!/bin/bash
# Perform broader searches for "density" and "charge_density"
echo "Searching for 'density' in Python files:"
rg --glob '*.py' '"density"'
echo -e "\nSearching for 'charge_density' in Python files:"
rg --glob '*.py' '"charge_density"'
echo -e "\nSearching for FluidDiagnostics usage:"
rg --glob '*.py' 'FluidDiagnostics'
Length of output: 11342
| @@ -36,7 +36,7 @@ | |||
| auto& By = model.state.electromag.B.getComponent(PHARE::core::Component::Y); | |||
| auto& Bz = model.state.electromag.B.getComponent(PHARE::core::Component::Z); | |||
|
|
|||
| auto& N = model.state.ions.density(); | |||
| auto& N = model.state.ions.chargeDensity(); | |||
Check notice
Code scanning / CodeQL
Unused local variable Note
| @@ -164,22 +163,25 @@ | |||
| template<typename... Args> | |||
| auto static _ions(Args&... args) | |||
| { | |||
| auto const& [Fi, Nibuffer, NiProtons, Vi, M, protons_M, pack] | |||
| auto const& [ionFlux, ionChargeDensity, ionMassDensity, protonParticleDensity, | |||
Check notice
Code scanning / CodeQL
Unused local variable Note test
| = std::forward_as_tuple(args...); | ||
| IonsT ions{createDict<dim>()["ions"]}; | ||
| { | ||
| auto const& [V, m, d, md] = ions.getCompileTimeResourcesViewList(); | ||
| d.setBuffer(&Nibuffer); | ||
| auto const& [V, m, d_c, d_m] = ions.getCompileTimeResourcesViewList(); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| @@ -313,25 +325,27 @@ | |||
| void setBuffers(Ions& ions) | |||
| { | |||
| { | |||
| auto const& [V, m, d, md] = ions.getCompileTimeResourcesViewList(); | |||
| auto const& [V, m, cd, md] = ions.getCompileTimeResourcesViewList(); | |||
Check notice
Code scanning / CodeQL
Unused local variable Note test
| md.setBuffer(&ionMassDensity); | ||
| } | ||
|
|
||
| auto& pops = ions.getRunTimeResourcesViewList(); | ||
| { | ||
| auto const& [F, M, d, particles] = pops[0].getCompileTimeResourcesViewList(); | ||
| d.setBuffer(&protonDensity); | ||
| auto const& [F, M, d, c, particles] = pops[0].getCompileTimeResourcesViewList(); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
| protons_M.set_on(M); | ||
| protonF.set_on(F); | ||
| particles.setBuffer(&protonPack); | ||
| } | ||
|
|
||
| { | ||
| auto const& [F, M, d, particles] = pops[1].getCompileTimeResourcesViewList(); | ||
| d.setBuffer(&alphaDensity); | ||
| auto const& [F, M, d, c, particles] = pops[1].getCompileTimeResourcesViewList(); |
Check notice
Code scanning / CodeQL
Unused local variable Note test
nicolasaunai
force-pushed
the
charge
branch
from
2191e39
to
75da612
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (17)
- pyphare/pyphare/pharein/diagnostics.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- src/amr/level_initializer/hybrid_level_initializer.hpp (1 hunks)
- src/amr/physical_models/hybrid_model.hpp (1 hunks)
- src/core/data/electrons/electrons.hpp (4 hunks)
- src/core/data/ions/ion_population/ion_population.hpp (5 hunks)
- src/core/data/ions/ions.hpp (9 hunks)
- src/core/numerics/interpolator/interpolator.hpp (3 hunks)
- src/core/numerics/ion_updater/ion_updater.hpp (4 hunks)
- src/core/numerics/moments/moments.hpp (2 hunks)
- src/diagnostic/detail/types/fluid.hpp (5 hunks)
- tests/core/data/electrons/test_electrons.cpp (8 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (12 hunks)
- tests/diagnostic/init.py (1 hunks)
- tests/diagnostic/test_diagnostics.hpp (4 hunks)
- tests/simulator/test_advance.py (5 hunks)
Files skipped from review as they are similar to previous changes (13)
- pyphare/pyphare/pharein/diagnostics.py
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/amr/physical_models/hybrid_model.hpp
- src/core/data/electrons/electrons.hpp
- src/core/data/ions/ion_population/ion_population.hpp
- src/core/numerics/interpolator/interpolator.hpp
- src/core/numerics/ion_updater/ion_updater.hpp
- src/core/numerics/moments/moments.hpp
- tests/core/data/electrons/test_electrons.cpp
- tests/core/numerics/ion_updater/test_updater.cpp
- tests/diagnostic/init.py
- tests/diagnostic/test_diagnostics.hpp
Additional context used
Path-based instructions (2)
src/core/data/ions/ions.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/diagnostic/detail/types/fluid.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Ruff
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
370-370: Local variable
coloris assigned to but never usedRemove assignment to unused variable
color(F841)
tests/simulator/test_advance.py
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[notice] 370-370: Unused local variable
Variable color is not used.
Additional comments not posted (35)
src/core/data/ions/ions.hpp (13)
44-45: LGTM!The changes to the constructor to initialize the new
chargeDensity_member variable and remove the unusedrho_variable are clear and consistent with the overall refactoring to separate charge density and mass density.
60-61: LGTM!The modifications to the
massDensity()accessor methods to remove references torho_are consistent with the removal of therho_variable.
63-64: LGTM!The addition of the
chargeDensity()accessor methods for the newchargeDensity_member variable is a clean way to provide access to the charge density data. TheNO_DISCARDattribute and the const/non-const overloads are good practices.
69-69: LGTM!The addition of the static
chargeDensityName()method to return the name of the charge density field is a clean way to centralize the field name. TheNO_DISCARDattribute is a good practice.
75-90: LGTM!The renaming of
computeDensitytocomputeChargeDensityand the modification to operate onchargeDensity_instead ofrho_are consistent with the overall refactoring to separate charge density and mass density.The method implementation is clean and efficient:
- Zeroing out
chargeDensity_before the summation is a good practice.- Using
std::transformwithstd::plus<Float>{}to sum up the densities is an efficient and expressive way to perform the summation.
Line range hint
91-108: LGTM!The modifications to
computeMassDensityto remove references torho_and exclusively handle mass density calculations are consistent with the overall refactoring.The method implementation is clean and efficient:
- Zeroing out
massDensity_before the summation is a good practice.- Using
std::transformwith a lambda to sum up the mass densities while accounting for the population mass is an efficient and expressive way to perform the summation.
113-113: LGTM!Updating
computeBulkVelocityto callcomputeMassDensity()instead ofcomputeDensity()is consistent with the renaming ofcomputeDensitytocomputeChargeDensity. Since the bulk velocity computation depends on the mass density, callingcomputeMassDensity()is the correct thing to do.
130-130: LGTM!Using
std::transformwith theplusMasslambda to sum up the mass-weighted velocities is an efficient and expressive way to perform the summation. Doing this for each velocity component (x, y, z) is the correct approach.Also applies to: 132-132, 134-134
138-138: LGTM!Dividing the mass-weighted velocities by the mass density to get the bulk velocity is the correct approach. Using
std::transformwithstd::divides<Float>{}to perform the division for each velocity component (x, y, z) is an efficient and expressive way to do it.Also applies to: 140-140, 142-142
175-176: LGTM!Adding the check for
chargeDensity_in theisUsablemethod is consistent with the addition of thechargeDensity_member variable. The method correctly checks the usability of all relevant member variables and populations, and returns the logical AND of all these checks.
189-190: LGTM!Adding the check for
chargeDensity_in theisSettablemethod is consistent with the addition of thechargeDensity_member variable. The method correctly checks the settability of all relevant member variables and populations, and returns the logical AND of all these checks.
214-215: LGTM!Including
chargeDensity_in the tuple returned bygetCompileTimeResourcesViewListis consistent with the addition of thechargeDensity_member variable. The method correctly returns a tuple of references to all relevant member variables usingstd::forward_as_tuple.
239-239: LGTM!The addition of the
chargeDensity_member variable to theIonsclass is consistent with the overall refactoring to separate charge density and mass density. Declaring it as a private member variable of typefield_typeis the correct design.src/diagnostic/detail/types/fluid.hpp (9)
149-150: LGTM!The change to include "charge_density" in the list of quantities for file creation is consistent with the overall theme of incorporating charge density as a new metric. This ensures that files are created for the "charge_density" quantity when necessary.
154-155: LGTM!The change to include "charge_density" and "mass_density" in the list of quantities for file creation under the "/ions/" tree is consistent with the overall theme of incorporating charge density as a new metric. This ensures that files are created for these quantities when necessary.
202-204: LGTM!The change to handle the "charge_density" quantity in the
getDataSetInfomethod for individual ion populations is consistent with the overall theme of incorporating charge density as a new metric. This ensures that the necessary dataset information is retrieved for the "charge_density" quantity when it is active for the current diagnostic.
212-213: LGTM!The change to handle the "charge_density" quantity in the
getDataSetInfomethod for the overall ion population under the "/ions/" tree is consistent with the overall theme of incorporating charge density as a new metric. This ensures that the necessary dataset information is retrieved for the "charge_density" quantity when it is active for the current diagnostic.
274-275: LGTM!The change to initialize the dataset for the "charge_density" quantity in the
initDataSetsmethod for individual ion populations is consistent with the overall theme of incorporating charge density as a new metric. This ensures that the dataset for the "charge_density" quantity is properly initialized when it is active for the current diagnostic.
283-284: LGTM!The change to initialize the dataset for the "charge_density" quantity in the
initDataSetsmethod for the overall ion population under the "/ions/" tree is consistent with the overall theme of incorporating charge density as a new metric. This ensures that the dataset for the "charge_density" quantity is properly initialized when it is active for the current diagnostic.
315-317: LGTM!The change to write the "charge_density" dataset in the
writemethod for individual ion populations is consistent with the overall theme of incorporating charge density as a new metric. This ensures that the "charge_density" dataset is properly written when it is active for the current diagnostic.
325-326: LGTM!The change to write the "charge_density" dataset in the
writemethod for the overall ion population under the "/ions/" tree is consistent with the overall theme of incorporating charge density as a new metric. This ensures that the "charge_density" dataset is properly written when it is active for the current diagnostic.
355-355: LGTM!The change to check and write attributes for the "charge_density" quantity in the
writeAttributesmethod for individual ion populations is consistent with the overall theme of incorporating charge density as a new metric. This ensures that the attributes for the "charge_density" quantity are properly written when it is active for the current diagnostic.pyphare/pyphare/pharesee/hierarchy/hierarchy.py (1)
Line range hint
356-409: Excellent improvements to the 2D patch visualization!The changes made to the
plot_2d_patchesmethod significantly enhance the visualization of 2D patches:
- Increasing the subplot size to 16x16 allows for a more spacious layout.
- Utilizing a 2D array (
ij) to represent the color values of the boxes improves performance and visual clarity.- Using
pcolormeshfor visualization provides better rendering compared to the previousPatchCollectionapproach.- Including optional x and y limits through
kwargsadds flexibility to the visualization.- Removing the tick settings simplifies the axis configuration.
These modifications greatly improve the functionality and usability of the plotting method. Well done!
Tools
Ruff
366-366: Local variable
miis assigned to but never usedRemove assignment to unused variable
mi(F841)
366-366: Local variable
mais assigned to but never usedRemove assignment to unused variable
ma(F841)
370-370: Local variable
coloris assigned to but never usedRemove assignment to unused variable
color(F841)
GitHub Check: CodeQL
[notice] 370-370: Unused local variable
Variable color is not used.tests/simulator/test_advance.py (12)
50-50: Replace mutable default argument withNone.The past review comment is still valid. Using mutable data structures as default arguments can lead to unexpected behavior. It's recommended to replace
model_init={"seed": 2}withmodel_init=Noneand handle the initialization within the function.Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
50-50: LGTM: The change tomodel_initimproves test reproducibility.Including a seed value in
model_initensures consistent initialization across test runs, which is beneficial for reproducibility.Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
155-155: Renaming diagnostic quantities is a good improvement.The changes from "density" to "charge_density" reflect a more accurate representation of the data being analyzed. The updates are consistent across the function and the filenames used for loading data.
Also applies to: 225-225, 234-234
292-292: Verify the reason for increasing the absolute tolerance.The absolute tolerance for the floating-point comparisons has been increased from
5.5e-15to2.5e-14. Please ensure that this change is intentional and aligns with the expected precision of the compared values.
296-312: The enhanced error handling is a valuable addition.Plotting the 2D patches with distinct colors for the compared datasets and saving the figure when an assertion fails greatly aids in debugging and understanding discrepancies in the test results.
Tools
Ruff
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
Line range hint
331-342: LGTM!The function logic is correct, and there are no significant changes that require attention.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
344-382: LGTM!The function logic is correct, and there are no significant changes that require attention.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
384-403: LGTM!The function logic is correct, and there are no significant changes that require attention.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
405-517: LGTM!The function logic is correct, and there are no significant changes that require attention. The complex logic for testing field coarsening via subcycles appears to be sound.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
519-625: LGTM!The function logic is correct, and there are no significant changes that require attention. The complex logic for testing field level ghosts via subcycles and coarser interpolation appears to be sound.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
627-668: LGTM!The function logic is correct, and there are no significant changes that require attention. Setting up the two simulations and comparing the field level ghosts is a valid approach for testing.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Line range hint
670-682: LGTM!The function logic is correct, and there are no significant changes that require attention. Verifying the presence and size of domain particle patch data on refined levels is a valid test.
Tools
Ruff
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
52-52: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
nicolasaunai
force-pushed
the
charge
branch
from
aa8894d
to
e10ff84
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (20)
- pyphare/pyphare/pharein/diagnostics.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2 hunks)
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py (1 hunks)
- pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py (3 hunks)
- src/amr/level_initializer/hybrid_level_initializer.hpp (1 hunks)
- src/amr/physical_models/hybrid_model.hpp (1 hunks)
- src/core/data/electrons/electrons.hpp (4 hunks)
- src/core/data/ions/ion_population/ion_population.hpp (5 hunks)
- src/core/data/ions/ions.hpp (9 hunks)
- src/core/numerics/interpolator/interpolator.hpp (3 hunks)
- src/core/numerics/ion_updater/ion_updater.hpp (4 hunks)
- src/core/numerics/moments/moments.hpp (2 hunks)
- src/diagnostic/detail/types/fluid.hpp (5 hunks)
- tests/core/data/electrons/test_electrons.cpp (8 hunks)
- tests/core/numerics/ion_updater/test_updater.cpp (12 hunks)
- tests/diagnostic/init.py (1 hunks)
- tests/diagnostic/test_diagnostics.hpp (4 hunks)
- tests/simulator/refinement/test_2d_10_core.py (3 hunks)
- tests/simulator/refinement/test_2d_2_core.py (3 hunks)
- tests/simulator/test_advance.py (5 hunks)
Files skipped from review as they are similar to previous changes (15)
- pyphare/pyphare/pharein/diagnostics.py
- pyphare/pyphare/pharesee/hierarchy/hierarchy_utils.py
- pyphare/pyphare_tests/test_pharesee/test_geometry_2d.py
- src/amr/level_initializer/hybrid_level_initializer.hpp
- src/amr/physical_models/hybrid_model.hpp
- src/core/data/electrons/electrons.hpp
- src/core/data/ions/ion_population/ion_population.hpp
- src/core/numerics/interpolator/interpolator.hpp
- src/core/numerics/ion_updater/ion_updater.hpp
- src/core/numerics/moments/moments.hpp
- tests/core/numerics/ion_updater/test_updater.cpp
- tests/diagnostic/init.py
- tests/diagnostic/test_diagnostics.hpp
- tests/simulator/refinement/test_2d_10_core.py
- tests/simulator/refinement/test_2d_2_core.py
Additional context used
Path-based instructions (2)
src/core/data/ions/ions.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/diagnostic/detail/types/fluid.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Ruff
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
383-383: Loop control variable
coordsnot used within loop bodyRename unused
coordsto_coords(B007)
tests/simulator/test_advance.py
50-50: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
GitHub Check: CodeQL
pyphare/pyphare/pharesee/hierarchy/hierarchy.py
[failure] 383-383: Suspicious unused loop iteration variable
For loop variable 'coords' is not used in the loop body.
Additional comments not posted (40)
src/core/data/ions/ions.hpp (13)
44-45: LGTM!The changes to the constructor improve clarity by explicitly initializing
chargeDensity_and removing the redundantrho_variable. This aligns with the overall goal of separating charge density from mass density.
60-61: LGTM!The modifications to the
massDensity()accessor methods are consistent with the removal ofrho_and the overall goal of separating mass density from charge density.
63-64: LGTM!The addition of the
chargeDensity()accessor methods is consistent with the goal of explicitly handling charge density and provides a clean way to access thechargeDensity_member variable.
69-69: LGTM!The addition of the
chargeDensityName()static method provides a consistent way to access the name of the charge density quantity, which can be useful for logging, debugging, or other purposes.
75-90: LGTM!The renaming of
computeDensitytocomputeChargeDensityand the modification to operate solely onchargeDensity_improve clarity and align with the goal of separating charge density from mass density. The method correctly accumulates the charge densities of all ion populations.
Line range hint
91-108: LGTM!The modifications to the
computeMassDensitymethod to remove references torho_and operate solely onmassDensity_are consistent with the overall changes and align with the goal of separating mass density from charge density. The method correctly accumulates the mass densities of all ion populations.
113-113: LGTM!Calling
computeMassDensityat the beginning of thecomputeBulkVelocitymethod ensures that themassDensity_member variable is up to date before computing the bulk velocity, which depends on the mass density.
130-134: LGTM!The transformations of the velocity components by adding the mass-weighted flux contributions from each ion population are consistent with the existing logic in the
computeBulkVelocitymethod and correctly accumulate the contributions into the bulk velocity components.
138-143: LGTM!Dividing the accumulated mass-weighted flux contributions by the mass density to obtain the final bulk velocity components is consistent with the existing logic in the
computeBulkVelocitymethod and correctly computes the bulk velocity.
175-176: LGTM!The modification to the
isUsablemethod to include a check forchargeDensity_is consistent with the addition ofchargeDensity_as a new member variable and the overall goal of separating charge density from mass density. It ensures that the usability ofchargeDensity_is also validated.
189-190: LGTM!The modifications to the
isSettablemethod to include checks for bothmassDensity_andchargeDensity_are consistent with the separation of mass density and charge density into distinct member variables. It ensures that the settability of both member variables is validated.
214-215: LGTM!The modification to the
getCompileTimeResourcesViewListmethod to include bothchargeDensity_andmassDensity_in the returned tuple is consistent with the separation of mass density and charge density into distinct member variables. It ensures that both member variables are included in the compile-time resources view list.
239-239: LGTM!The addition of
chargeDensity_as a private member variable is consistent with the overall changes to separate charge density from mass density. It ensures proper encapsulation of thechargeDensity_member variable within theIonsclass.src/diagnostic/detail/types/fluid.hpp (5)
149-155: LGTM!The change from "density" to "charge_density" aligns with the overall emphasis on charge density in the diagnostic framework. The rest of the function remains unchanged and looks good.
202-204: LGTM!The change from "density" to "charge_density" aligns with the overall emphasis on charge density in the diagnostic framework. The rest of the function remains unchanged and looks good.
274-284: LGTM!The change from "density" to "charge_density" aligns with the overall emphasis on charge density in the diagnostic framework. The rest of the function remains unchanged and looks good.
315-326: LGTM!The change from "density" to "charge_density" aligns with the overall emphasis on charge density in the diagnostic framework. The rest of the function remains unchanged and looks good.
355-355: LGTM!The change from "density" to "charge_density" aligns with the overall emphasis on charge density in the diagnostic framework. The rest of the function remains unchanged and looks good.
tests/core/data/electrons/test_electrons.cpp (13)
129-130: LGTM!The addition of
dimandinterpas static constexpr variables improves code readability and maintainability.
151-151: LGTM!The renaming of
Mandprotons_MtoionTensorandprotonTensorimproves code clarity and maintainability.
153-153: LGTM!The renaming of
NibufferandNiProtonstoionChargeDensity,ionMassDensity,protonParticleDensity, andprotonChargeDensityimproves code clarity and maintainability.
171-173: LGTM!The
_ionsfunction has been updated to use the new variable names for ion charge density, ion mass density, and ion momentum tensor. The logic for setting buffers and accessing resources is correct.Also applies to: 175-175
180-184: LGTM!The
_ionsfunction has been updated to use the new variable names for proton particle density, proton charge density, and proton momentum tensor. The logic for setting buffers and accessing resources is correct.
196-197: LGTM!The constructor of
ElectronsTesthas been updated to initializeionTensorandprotonTensorwith the new variable names. The initialization logic is correct.
198-205: LGTM!The constructor of
ElectronsTesthas been updated to initializeionChargeDensity,ionMassDensity,protonParticleDensity, andprotonChargeDensitywith the new variable names. The initialization logic is correct.
247-247: LGTM!The
fillfunction has been updated to useionChargeDensityinstead ofNibuffer. The logic for filling the ion charge density is correct.
276-277: LGTM!The
fillfunction has been updated to useionChargeDensityinstead ofNibuffer. The logic for filling the ion charge density is correct.
323-325: LGTM!The
fillfunction has been updated to useionChargeDensityinstead ofNibuffer. The logic for filling the ion charge density is correct.
373-373: LGTM!The test case has been updated to use
ions.chargeDensity()instead ofions.density()to access the ion charge density. This aligns with the new naming conventions.
207-208: Verify the constructor changes.The constructor of
ElectronsTesthas been updated to pass the new variable names to the_ionsfunction. Ensure that the constructor logic is still correct after the variable name changes.Run the following script to verify the constructor:
Verification successful
Constructor changes verified successfully
The ElectronsTest constructor has been correctly updated to use the new variable names. All the mentioned variables (F, ionChargeDensity, ionMassDensity, protonParticleDensity, protonChargeDensity, Vi, ionTensor, protonTensor) are being passed to the _ions function in the correct order. The constructor logic remains consistent with the changes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the constructor of `ElectronsTest` is updated to use the new variable names. # Test: Search for the constructor. Expect: The constructor should use the new variable names. rg --type cpp $'ElectronsTest\(\)' tests/core/data/electrons/test_electrons.cppLength of output: 100
Script:
#!/bin/bash # Description: Verify the constructor of `ElectronsTest` is updated to use the new variable names. # Search for the ElectronsTest constructor with 20 lines of context after the match rg --type cpp -A 20 'ElectronsTest\(\)' tests/core/data/electrons/test_electrons.cppLength of output: 1515
166-167: Verify the variable name changes in the_ionsfunction.The
_ionsfunction has been updated to use the new variable names for ion and proton properties. Ensure that the function logic is still correct after the variable name changes.Run the following script to verify the variable usage:
Verification successful
Variable name changes in the
_ionsfunction have been implemented correctly.The verification process shows that the old variable names (Nibuffer, NiProtons, M, protons_M) are not present in the file. The new naming convention, including "protons_" prefixes for proton-related variables, is consistently used throughout the visible code. While we couldn't locate the exact
_ionsfunction, the overall code structure suggests that the variable name changes have been applied correctly.Note: The
_ionsfunction might have been renamed or moved to a different file. If you want to verify its specific implementation, you may need to manually locate the function in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all variable usages in `_ions` function are updated to the new names. # Test: Search for the old variable names. Expect: No occurrences of the old names. rg --type cpp $'Nibuffer|NiProtons|M|protons_M' tests/core/data/electrons/test_electrons.cppLength of output: 971
Script:
#!/bin/bash # Description: Verify the _ions function and search for potential remnants of old variable names # Display the entire _ions function echo "=== _ions function content ===" ast-grep --lang cpp --pattern $'_ions($$$) { $$$ }' tests/core/data/electrons/test_electrons.cpp # Search for old variable names in comments or string literals echo -e "\n=== Old variable names in comments or strings ===" rg --type cpp -i '(Nibuffer|NiProtons|protons_M)' tests/core/data/electrons/test_electrons.cpp # Flexible search for partial matches of old variable names echo -e "\n=== Partial matches of old variable names ===" rg --type cpp -i '(Ni.*buffer|Ni.*rotons|protons_.*|(?<![a-zA-Z])M(?![a-zA-Z]))' tests/core/data/electrons/test_electrons.cppLength of output: 1001
Script:
#!/bin/bash # Description: Retry verifying the _ions function and search for potential remnants of old variable names # Display the _ions function content with a more flexible pattern echo "=== _ions function content ===" ast-grep --lang cpp --pattern 'ions($$$) { $$$ }' tests/core/data/electrons/test_electrons.cpp # Search for old variable names directly echo -e "\n=== Direct search for old variable names ===" grep -n -i 'Nibuffer\|NiProtons\|protons_M' tests/core/data/electrons/test_electrons.cpp # Simpler search for partial matches of old variable names echo -e "\n=== Partial matches of old variable names (simplified) ===" grep -n -i 'Ni.*buffer\|Ni.*rotons\|protons_\|[^a-zA-Z]M[^a-zA-Z]' tests/core/data/electrons/test_electrons.cppLength of output: 1413
pyphare/pyphare/pharesee/hierarchy/hierarchy.py (4)
356-357: LGTM!The import statement for
Rectangleis correct and necessary for the code that follows.
368-374: Looks good!The initialization of the figure, axis, and data structures is correctly implemented based on the level domain box.
386-397: Nicely done!The code segment correctly plots the
ijarray usingpcolormesh, sets the tick marks, and overlays the patch rectangles on the plot. The visual customization of the rectangles enhances the clarity of the patch boundaries.
405-408: Good addition!The code segment provides a convenient way to set the x-axis and y-axis limits based on the optional
"xlim"and"ylim"keyword arguments. This allows for flexibility in controlling the visible range of the plot.tests/simulator/test_advance.py (5)
155-155: LGTM!The change from "density" to "charge_density" is consistent with the list of alterations and looks good.
225-225: LGTM!The change from "ions_density.h5" to "ions_charge_density.h5" is consistent with the list of alterations and looks good.
Also applies to: 234-234
292-292: LGTM!The change in assertion tolerance from
5.5e-15to2.5e-14is consistent across the code and looks good.Increasing the absolute tolerance suggests a shift in the expected precision of the comparisons being made, potentially accommodating for larger discrepancies in the floating-point comparisons.
Also applies to: 296-298
299-311: Great error handling!The error handling code enhances the debugging process by visualizing the differences between the compared slices when an assertion fails. Plotting the 2D patches with distinct colors helps in identifying and understanding the discrepancies in the test results. Saving the figure allows for further analysis and investigation of the failures.
This error handling mechanism aids in debugging and understanding the test failures effectively.
Line range hint
1-313: Review complete!The
getHierarchyfunction has been thoroughly reviewed, and all the changes have been approved or addressed with appropriate suggestions. No further changes or issues are observed.Tools
Ruff
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
| xlim=(-5, 50), | ||
| ylim=(-5, 50), | ||
| ) | ||
| fig.savefig(f"pd1.png") |
There was a problem hiding this comment.
Remove extraneous f prefix.
The f-string on line 312 doesn't contain any placeholders. Remove the f prefix to fix this issue.
-fig.savefig(f"pd1.png")
+fig.savefig("pd1.png")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fig.savefig(f"pd1.png") | |
| fig.savefig("pd1.png") |
Tools
Ruff
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tests/simulator/test_advance.py (1)
302-312: Helpful visualization for debugging!Plotting the 2D patches with distinct colors is a great way to visualize and investigate the discrepancies when an assertion fails. Saving the figure enables further analysis of the issues.
One minor suggestion:
Remove the extraneous
fprefix in thesavefigcall on line 312 since it's not an f-string.-fig.savefig(f"pd1.png") +fig.savefig("pd1.png")Tools
Ruff
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py (2 hunks)
- tests/simulator/test_advance.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyphare/pyphare/pharesee/hierarchy/hierarchy.py
Additional context used
Ruff
tests/simulator/test_advance.py
312-312: f-string without any placeholders
Remove extraneous
fprefix(F541)
Additional comments not posted (4)
tests/simulator/test_advance.py (4)
155-155: LGTM!The renaming of the diagnostic quantity from "density" to "charge_density" improves clarity and better reflects the physical meaning of the data.
225-225: Looks good!The filename changes from "ions_density.h5" to "ions_charge_density.h5" maintain consistency with the diagnostic quantity renaming.
Also applies to: 234-234
292-292: Verify the rationale for increasing the assertion tolerance.The absolute tolerance for floating-point comparisons has been increased from 5.5e-15 to 2.5e-14. Please ensure that this change is justified and does not mask any underlying issues.
296-300: Nice addition!The enhanced error handling mechanism provides a useful way to visualize and analyze the differences between the slices when an assertion fails. The adjustments to the error indices ensure proper mapping to the original data space.
|
closing for @rochSmets to make a new PR with his last edits |
Summary by CodeRabbit
New Features
Bug Fixes
Tests