tagging thresholds from python #879
Conversation
WalkthroughWalkthroughThe recent updates enhance the configurability of the simulation framework by introducing a new "tagging" refinement method. Key changes include a customizable tagging threshold, a shift to a dictionary-based approach for model and method specifications, and improvements to the testing framework to validate both valid and invalid configurations. These enhancements aim to make the simulation more flexible and adaptable for various scenarios. Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
src/amr/tagging/tagger_factory.hpp (1)
Ensure Proper Handling of Dictionary Keys
The code does not currently check for the presence of the "model" and "method" keys before accessing them. This could lead to runtime errors if these keys are missing from the dictionary. Consider adding checks or default values to ensure robustness.
- File:
src/amr/tagging/tagger_factory.hpp- Lines: 21-38
Analysis chain
Line range hint
21-38:
Ensure proper handling of dictionary keys.The transition to using
PHAREDictenhances flexibility. However, ensure that the keys"model"and"method"are always present in the dictionary to avoid runtime errors. Consider adding checks or default values for robustness.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the dictionary keys "model" and "method" are used correctly throughout the codebase. # Test: Search for dictionary key usage. Expect: Consistent usage of "model" and "method" keys. rg --type cpp --word-regexp 'dict\["(model|method)"\]'Length of output: 591
Script:
#!/bin/bash # Search for any error handling or conditional checks related to the "model" and "method" keys in the codebase. rg --type cpp 'if\s*\(.*dict\["(model|method)"\].*\)'Length of output: 55
pyphare/pyphare/pharein/__init__.py (1)
225-232: Consider making hardcoded parameters configurable.The parameters
"HybridModel"and"default"are currently hardcoded. Consider making these configurable to enhance flexibility, especially if multiple models or methods are anticipated in future iterations.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (3 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (6 hunks)
- src/amr/tagging/tagger_factory.hpp (3 hunks)
- src/simulator/simulator.hpp (1 hunks)
- tests/amr/tagging/test_tagging.cpp (1 hunks)
Additional context used
Path-based instructions (3)
src/amr/tagging/tagger_factory.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/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (7)
src/amr/tagging/default_hybrid_tagger_strategy.hpp (2)
65-66: Utilizethreshold_consistently intagmethod.The
tagmethod now uses thethreshold_member variable, which aligns with the new configurability. Ensure thatthreshold_is correctly set for all use cases to maintain expected behavior.Also applies to: 91-92, 122-125
21-25: Constructor improvement for configurability.The addition of a constructor that initializes
threshold_from a dictionary enhances flexibility. Ensure that the dictionary always contains a valid"threshold"key to prevent exceptions.tests/amr/tagging/test_tagging.cpp (2)
24-32: Test casefromFactoryValidensures valid tagger creation.The use of
PHAREDictto pass parameters improves clarity and maintainability. The test effectively verifies valid tagger creation.
34-41: Test casefromFactoryInvalidenhances robustness.This new test case ensures that the factory correctly handles invalid configurations by returning
nullptr. It effectively tests the error handling mechanism.src/simulator/simulator.hpp (1)
272-272: Verify the correctness of the dictionary key.The change from a hardcoded string to a dynamic value from
dict["tagging"]enhances flexibility. Ensure that the key"tagging"is consistently used and correctly populated in all relevant contexts.Verification successful
The "tagging" key is used consistently across the codebase.
The change to use
dict["tagging"]insimulator.hppaligns with existing patterns in both test and source files, confirming its correctness and consistency. No issues found with the usage of the "tagging" key.
- Files with "tagging" key usage:
tests/simulator/test_init_periodicity.pytests/simulator/test_run.pytests/simulator/test_load_balancing.pytests/simulator/test_restarts.pysrc/simulator/simulator.hppsrc/phare/phare_init.pysrc/amr/wrappers/integrator.hpppyphare/pyphare/pharein/simulation.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the "tagging" key in the codebase to ensure consistency and correctness. # Test: Search for occurrences of the "tagging" key in the codebase. Expect: Consistent usage in relevant contexts. rg --type cpp --type python '"tagging"'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of the "tagging" key in the codebase to ensure consistency and correctness. # Test: Search for occurrences of the "tagging" key in the codebase. Expect: Consistent usage in relevant contexts. rg --type cpp --type py '"tagging"'Length of output: 1593
pyphare/pyphare/pharein/simulation.py (2)
607-607: Verify the appropriateness of the defaulttagging_threshold.The addition of
tagging_thresholdwith a default value of0.1enhances configurability. Ensure that this default value is suitable for typical use cases.
809-813: Ensure accuracy of the updated documentation.The docstring now includes information about the "tagging" refinement type. Verify that the documentation accurately describes the implementation and usage of the
tagging_threshold.Verification successful
Documentation accurately reflects the implementation.
The "tagging_threshold" is correctly documented and implemented in the codebase, ensuring consistency between the docstring and the actual usage. The parameter is used to set a default value for tagging cells for refinement, as described.
pyphare/pyphare/pharein/simulation.py: Mentions and uses "tagging_threshold".pyphare/pyphare/pharein/__init__.py: References "tagging_threshold".Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the accuracy of the updated documentation for the "tagging" refinement type. # Test: Check for consistency between the documentation and the implementation of the "tagging" refinement type. rg --type python 'tagging_threshold'Length of output: 67
Script:
#!/bin/bash # Description: Verify the accuracy of the updated documentation for the "tagging" refinement type. # Test: Check for consistency between the documentation and the implementation of the "tagging" refinement type. rg --type py 'tagging_threshold'Length of output: 458
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (3 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (6 hunks)
- src/amr/tagging/tagger_factory.hpp (3 hunks)
- src/simulator/simulator.hpp (1 hunks)
- tests/amr/tagging/test_tagging.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- pyphare/pyphare/pharein/init.py
- pyphare/pyphare/pharein/simulation.py
- src/amr/tagging/default_hybrid_tagger_strategy.hpp
- src/amr/tagging/tagger_factory.hpp
- tests/amr/tagging/test_tagging.cpp
Additional context used
Path-based instructions (1)
src/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (1)
src/simulator/simulator.hpp (1)
272-272: Improved configurability with dynamic tagging.The change to use
dict["tagging"]for initializinghybridTagger_enhances the simulator's flexibility by allowing external configuration of the tagging mechanism. Ensure that thedictalways contains the "tagging" key to avoid runtime errors.Verification successful
Verification complete: "tagging" key access is safeguarded.
The code includes a check for the presence of the "tagging" key in the dictionary, ensuring that the access is safe and preventing potential runtime errors. This confirms that the change is implemented with appropriate safeguards.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all uses of the `dict` object include a "tagging" key. # Test: Search for all dictionary accesses to ensure "tagging" is always present. rg --type cpp -A 5 $'dict\\["tagging"\\]'Length of output: 500
Script:
#!/bin/bash # Description: Check for any initialization or checks for "tagging" key in the dictionary. # Test: Search for any initialization or checks for "tagging" in the source files. rg --type cpp -A 5 'dict.*tagging'Length of output: 1419
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (3 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (6 hunks)
- src/amr/tagging/tagger_factory.hpp (3 hunks)
- src/simulator/simulator.hpp (1 hunks)
- tests/amr/tagging/test_tagging.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- pyphare/pyphare/pharein/init.py
- pyphare/pyphare/pharein/simulation.py
- src/amr/tagging/default_hybrid_tagger_strategy.hpp
- src/amr/tagging/tagger_factory.hpp
- src/simulator/simulator.hpp
- tests/amr/tagging/test_tagging.cpp
| @@ -269,8 +269,16 @@ void Simulator<dim, _interp, nbRefinedPart>::hybrid_init(initializer::PHAREDict | |||
| multiphysInteg_->registerAndSetupMessengers(messengerFactory_); | |||
|
|
|||
| // hard coded for now, should get some params later from the dict | |||
| auto hybridTagger_ = amr::TaggerFactory<PHARETypes>::make("HybridModel", "default"); | |||
| multiphysInteg_->registerTagger(0, maxLevelNumber_ - 1, std::move(hybridTagger_)); | |||
| if (dict["simulation"]["AMR"]["refinement"].contains("tagging")) | |||
There was a problem hiding this comment.
I found it weird that we create the Tagger even though we may not be tagging.
And with this new PR it would not work anyway (because the Tagger now takes the threshold from the dict it's now given, and there would not be any threshold in that dict if tagging is not specified by the user)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (6 hunks)
Additional context used
Path-based instructions (1)
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.
Additional comments not posted (6)
src/amr/tagging/default_hybrid_tagger_strategy.hpp (6)
9-9: LGTM!The inclusion of the "initializer/data_provider.hpp" header file is approved as it aligns with the shift towards a more configurable design.
21-24: LGTM!The constructor changes are approved for the following reasons:
- The use of
initializer::PHAREDictas a parameter allows for dynamic configuration of thethreshold_value.- The
cppdict::get_valuefunction is used with a default value of 0.1, addressing the suggestion from the previous review.The comment from the previous review suggesting the use of
cppdict::get_valuehas been addressed and can be marked as resolved.
28-28: LGTM!The addition of the
threshold_member variable is approved as it allows the threshold value to be set externally and provides a reasonable default value.
64-64: LGTM!The change in the
tagmethod to use thethreshold_member variable instead of the hardcoded value is approved. This enhances the flexibility of the tagging strategy by allowing different threshold values to be specified at runtime.
90-90: LGTM!The change in the
tagmethod to use thethreshold_member variable instead of the hardcoded value is approved. This enhances the flexibility of the tagging strategy by allowing different threshold values to be specified at runtime.
121-121: LGTM!The change in the
tagmethod to use thethreshold_member variable instead of the hardcoded value is approved. This enhances the flexibility of the tagging strategy by allowing different threshold values to be specified at runtime.
|
you need to rebase master @nicolasaunai |
nicolasaunai
force-pushed
the
threshold
branch
from
629ba5e
to
cc08e2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- pyphare/pyphare/pharein/init.py (1 hunks)
- pyphare/pyphare/pharein/simulation.py (3 hunks)
- src/amr/tagging/default_hybrid_tagger_strategy.hpp (6 hunks)
- src/amr/tagging/tagger_factory.hpp (3 hunks)
- src/simulator/simulator.hpp (1 hunks)
- tests/amr/tagging/test_tagging.cpp (1 hunks)
Additional context used
Path-based instructions (3)
src/amr/tagging/tagger_factory.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/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (14)
src/amr/tagging/tagger_factory.hpp (4)
12-12: LGTM!The new header file is required for the
PHAREDicttype.
21-21: LGTM!The updated function signature enhances the flexibility and maintainability of the code by:
- Reducing the number of parameters.
- Allowing for more complex configurations through the use of
PHAREDict.
25-25: LGTM!The function definition has been updated to match the new signature.
Line range hint
27-38: LGTM!The changes in the function body enhance the flexibility and maintainability of the code by:
- Extracting the model and method names from the dictionary, allowing for more complex configurations.
- Passing the
dictto theHTSconstructor, potentially making theDefaultHybridTaggerStrategybehavior more configurable.These changes reflect a shift towards a more structured approach to managing configurations.
src/amr/tagging/default_hybrid_tagger_strategy.hpp (4)
9-9: Verify the purpose and usage of the included header file.The inclusion of the
initializer/data_provider.hppheader file suggests a dependency on external data initialization.Please verify that this header file is being used correctly and for the intended purpose within the
DefaultHybridTaggerStrategyclass.
21-24: LGTM!The changes to the constructor allow for dynamic configuration of the
threshold_value based on the provided dictionary. Usingcppdict::get_valuewith a default value ensures a fallback value if the key is not present in the dictionary.
28-28: LGTM!Moving the threshold to a member variable allows for external configuration via the constructor. The default value of
0.1ensures a reasonable value if not provided through the constructor.
64-64: LGTM!The usage of the
threshold_member variable is consistent with the changes made to introduce it as a configurable parameter.Also applies to: 90-90, 121-121
tests/amr/tagging/test_tagging.cpp (2)
24-33: LGTM!The changes to the
fromFactoryValidtest case are approved:
- The name change improves clarity by focusing on valid input scenarios.
- Using a
PHAREDictto pass parameters enhances flexibility and readability of the test setup.- The test correctly checks that the returned pointer is not null for valid input.
35-43: Great addition!The new
fromFactoryInvalidtest case is approved:
- It improves the robustness of the testing framework by validating the behavior of the factory under erroneous conditions.
- Using a
PHAREDictto pass invalid parameters is consistent with the approach in thefromFactoryValidtest case.- The test correctly checks that the returned pointer is null for invalid input.
src/simulator/simulator.hpp (1)
272-280: LGTM!The changes introduce a conditional logic structure that enhances the flexibility of the tagging mechanism. The
hybridTagger_is dynamically created using the parameters specified in the dictionary, allowing different tagging methods to be employed based on user-defined settings. This improves the adaptability of the simulation setup without altering the codebase.The changes are well-structured, follow the existing coding style, and do not introduce any performance or security issues.
pyphare/pyphare/pharein/simulation.py (3)
607-607: LGTM!The code change is approved.
681-681: LGTM, but verify the default value.The code change is approved.
However, please verify if the default value of 0.1 for
tagging_thresholdis suitable for all use cases.
809-813: LGTM!The changes to the docstring are approved. The updated docstring provides clarity on the new
taggingrefinement type and how it interacts with the existingrefinement_boxesmethod.
| # the two following params are hard-coded for now | ||
| # they will become configurable when we have multi-models or several methods | ||
| # per model | ||
| add_string("simulation/AMR/refinement/tagging/model", "HybridModel") | ||
| add_string("simulation/AMR/refinement/tagging/method", "default") | ||
| add_double( | ||
| "simulation/AMR/refinement/tagging/threshold", simulation.tagging_threshold | ||
| ) |
There was a problem hiding this comment.
Introducing a dictionary-based approach for refinement parameters
The changes introduce a more structured approach to managing simulation parameters related to the "tagging" refinement method. The use of hard-coded values for the "model" and "method" parameters suggests that this is a preparatory step for future enhancements.
Consider the following suggestions for further improvement:
-
Are there plans to support multiple models or methods in the future? If so, it would be beneficial to replace the hard-coded values with configurable options.
-
Verify that the
simulation.tagging_thresholdattribute is properly initialized before use to avoid potential issues. -
Consider adding validation checks for the "model" and "method" parameters to ensure they are valid values based on the supported options.
By addressing these points, the code will be more maintainable and extensible as new features are added.
|
the macos issue fix was merged already, I don't think you need to bother rebasing for that |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation