refactor!: Accumulated material changes #4077
Conversation
WalkthroughRevisions many, this pull request brings. Explicit defaults now set for material properties using factory methods— Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (21)
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
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
andiwand
force-pushed
the
accumulated-material-changes
branch
from
3930ce6 to
5bc6587
Compare
![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
🧹 Nitpick comments (2)
Core/src/Material/SurfaceMaterialMapper.cpp (1)
284-370: Improved enum usage with proper scoping, hmm!MappingType enums now explicitly scoped are. Before, raw enum values might have been used. Better practice this is, yes.
Core/src/Material/VolumeMaterialMapper.cpp (1)
448-452: Factory method for vacuum material, introduced it was!-auto properties = Acts::MaterialSlab(Acts::Material(), vacuumThickness); +auto properties = MaterialSlab::Vacuum(vacuumThickness);Better code this is! Factory method for vacuum material, more explicit intent reveals. Aligns with PR objective to introduce explicit factory functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
Core/include/Acts/Material/AccumulatedMaterialSlab.hpp(1 hunks)Core/include/Acts/Material/AccumulatedVolumeMaterial.hpp(1 hunks)Core/include/Acts/Material/HomogeneousSurfaceMaterial.hpp(1 hunks)Core/include/Acts/Material/ISurfaceMaterial.hpp(2 hunks)Core/include/Acts/Material/Material.hpp(4 hunks)Core/include/Acts/Material/MaterialInteraction.hpp(1 hunks)Core/include/Acts/Material/MaterialSlab.hpp(4 hunks)Core/include/Acts/Material/ProtoSurfaceMaterial.hpp(1 hunks)Core/include/Acts/Material/ProtoVolumeMaterial.hpp(1 hunks)Core/include/Acts/Propagator/EigenStepperDenseExtension.hpp(1 hunks)Core/include/Acts/Propagator/detail/PointwiseMaterialInteraction.hpp(2 hunks)Core/include/Acts/Propagator/detail/VolumeMaterialInteraction.hpp(2 hunks)Core/include/Acts/Utilities/Grid.hpp(0 hunks)Core/include/Acts/Utilities/GridIterator.ipp(1 hunks)Core/src/Geometry/PortalShell.cpp(1 hunks)Core/src/Material/AccumulatedSurfaceMaterial.cpp(1 hunks)Core/src/Material/AverageMaterials.cpp(1 hunks)Core/src/Material/Interactions.cpp(9 hunks)Core/src/Material/Material.cpp(1 hunks)Core/src/Material/MaterialSlab.cpp(2 hunks)Core/src/Material/ProtoVolumeMaterial.cpp(1 hunks)Core/src/Material/SurfaceMaterialMapper.cpp(15 hunks)Core/src/Material/VolumeMaterialMapper.cpp(19 hunks)Examples/Io/Root/src/RootMaterialDecorator.cpp(1 hunks)Examples/Io/Root/src/RootMaterialWriter.cpp(2 hunks)Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp(1 hunks)Plugins/Json/src/MaterialJsonConverter.cpp(6 hunks)Tests/UnitTests/Core/Geometry/PortalTests.cpp(3 hunks)Tests/UnitTests/Core/Material/AccumulatedMaterialSlabTests.cpp(4 hunks)Tests/UnitTests/Core/Material/AccumulatedVolumeMaterialTests.cpp(2 hunks)Tests/UnitTests/Core/Material/AverageMaterialsTests.cpp(16 hunks)Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp(5 hunks)Tests/UnitTests/Core/Material/GridSurfaceMaterialTests.cpp(10 hunks)Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp(1 hunks)Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp(1 hunks)Tests/UnitTests/Core/Material/InteractionsTests.cpp(1 hunks)Tests/UnitTests/Core/Material/MaterialGridHelperTests.cpp(4 hunks)Tests/UnitTests/Core/Material/MaterialSlabTests.cpp(1 hunks)Tests/UnitTests/Core/Material/MaterialTests.cpp(2 hunks)Tests/UnitTests/Core/Material/VolumeMaterialMapperTests.cpp(4 hunks)Tests/UnitTests/Plugins/Json/MaterialJsonConverterTests.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- Core/include/Acts/Utilities/Grid.hpp
✅ Files skipped from review due to trivial changes (2)
- Core/include/Acts/Utilities/GridIterator.ipp
- Core/src/Geometry/PortalShell.cpp
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: macos
🔇 Additional comments (124)
Core/src/Material/AccumulatedSurfaceMaterial.cpp (1)
143-143: Initializer change, good it is!From default
MaterialSlab()constructor to explicitMaterialSlab::Nothing()factory function, changed this has been. Clear intention for empty material slabs, this shows. With the PR's introduction of explicit factory functions, aligned this change is.Tests/UnitTests/Core/Material/MaterialSlabTests.cpp (1)
40-40: Method name change, more accurate it is!From
averageLayerstocombineLayers, renamed this method has been. Better represents the actual operation performed, this new name does. Not merely averaging layers but combining them in specific way, this function does. With PR objectives to rename methods for clarity, aligned perfectly this is.Core/include/Acts/Material/MaterialInteraction.hpp (1)
90-90: Explicit initialization, wise decision this is!Default initialized was
materialSlabbefore, now explicitly set toMaterialSlab::Nothing()it is. Clear intention and consistent state across codebase, this change provides. No longer rely on implicit behavior, we must. Explicit factory methods, use we should!Core/include/Acts/Material/ProtoVolumeMaterial.hpp (1)
72-72: Explicit vacuum initialization, clarity it brings!Default constructed was
m_materialbefore, now initialized withMaterial::Vacuum()it is. Part of broader effort to use explicit factory functions for vacuum materials, this change is. Better expressing intent in code, it does. Mmm, yes!Tests/UnitTests/Core/Material/ISurfaceMaterialTests.cpp (1)
39-39: Clearer initialization with factory method, I see.Better readability this brings, hmm. Default constructor replaced by explicit factory method MaterialSlab::Nothing(), the intent clearer it makes. For representing vacuum state, proper this is.
Tests/UnitTests/Core/Geometry/PortalTests.cpp (3)
217-218: Good use of factory method for vacuum material, yes.MaterialSlab::Nothing() now used to initialize vacuum material, clearer this makes the code's intent. The comment "// vacuum" helpful addition it is. Consistent with other material initialization changes throughout the codebase, this is.
328-329: Consistent use of MaterialSlab::Nothing(), approve I do.Same pattern as before, using factory method instead of direct initialization. Clarity it brings to material representation, hmm.
499-500: Consistency maintained throughout tests, good this is.Again the factory method used, maintaining consistency across codebase. Makes code more maintainable and intent clearer, it does.
Examples/Io/Root/src/RootMaterialDecorator.cpp (1)
160-161: Improved material matrix initialization, I see.Updated to use MaterialSlab::Nothing() for matrix elements, proper default state it ensures. Part of larger refactoring effort to make vacuum material handling explicit, this is.
Core/src/Material/AverageMaterials.cpp (1)
54-54: Better vacuum representation with Material::Vacuum(), wise choice this is.Changed from likely default-constructed Material to explicit Material::Vacuum() in zero molar amount case. More precise intent it communicates. Aligned with PR objectives to create explicit factory functions for vacuum material, this change is.
Core/src/Material/ProtoVolumeMaterial.cpp (1)
13-14: Improved initialization pattern, mmm, yes!Constructor initialization list simplified it has been. Member variable
m_materialnow defaults toMaterial::Vacuum()directly in the class definition it does. More robust and predictable code this creates, yes.Core/include/Acts/Material/AccumulatedVolumeMaterial.hpp (1)
31-31: Explicit initialization with factory function, wise choice is!Initialize
m_averagewithMaterialSlab::Nothing()you now do. Clear and explicit initial state this provides. Documentation in line 27 matches this behavior it does - "Vacuum properties if no matter has been accumulated yet." Hmm, consistent this is!Core/include/Acts/Propagator/EigenStepperDenseExtension.hpp (1)
39-39: Default to Material::Vacuum(), good practice this is!Explicit initialization using factory function, instead of default constructor, stronger code makes. Prevent unexpected behaviors this does. Track with PR objectives this change does, for vacuum material factory functions.
Core/include/Acts/Material/ProtoSurfaceMaterial.hpp (1)
108-108: Initialized material, more robust code makes!Used factory function
MaterialSlab::Nothing()you have, instead of uninitialized state. Consistent with other changes in pull request this is. Return this value inmaterialSlab()method you do, so initialize it properly, important it is! Clear intention this shows to readers of your code.Core/include/Acts/Material/ISurfaceMaterial.hpp (4)
151-152: Check for vacuum presence, not its absence.Changed condition now checks if material is not vacuum, hmm? Clearer intent this provides. Consistent with PR's spirit of refactoring.
154-155: Use factory method, wise choice this is.
MaterialSlab::Nothing()factory method now used instead of default constructor. More explicit and readable, makes the code, yes.
166-167: Consistent approach to vacuum detection, I sense.Same change as above, checking for non-vacuum state. Throughout codebase, maintain consistency this does.
169-170: Same factory method used here, good.
MaterialSlab::Nothing()again used. Consistent approach to representing empty material states, this ensures.Tests/UnitTests/Core/Material/InteractionsTests.cpp (1)
105-105: Explicit factory function for vacuum, more idiomatic this is.Replace direct constructor call with dedicated
MaterialSlab::Vacuum(x)factory method. Improve code readability and intent, this does. Aligns with PR objective, it does, yes.Examples/Io/Root/src/RootMaterialWriter.cpp (2)
300-300: Condition now reflects intent more clearly.Changed from checking validity to checking if not vacuum. More intention-revealing, the code becomes. At one with the Force, this change is.
314-314: Consistent with previous change, this is.Same pattern applied to 2D grid volume code. Throughout codebase, consistency in material validation approach, this maintains.
Tests/UnitTests/Core/Material/HomogeneousSurfaceMaterialTests.cpp (1)
71-71: Factory method for empty material slab, clearer this makes.Use
MaterialSlab::Nothing()instead of default constructor, yes. Improve code readability and express intent clearly, this does. Match approach in other files, it does.Fatras/include/ActsFatras/Kernel/detail/SimulationActor.hpp (1)
162-162: Changed condition from validity check to vacuum check, mmm!From checking if slab is valid, to checking if slab is not vacuum, changed the condition has. Clearer intent this shows, about physics of the simulation it speaks. Appropriately renamed method this is, yes.
Core/include/Acts/Material/HomogeneousSurfaceMaterial.hpp (1)
91-91: Default initialization added, strong the code becomes!Initialized the material variable is now!
MaterialSlab::Nothing()factory function used, mmm. Undefined state avoided has been. Clear default state for material established, proper initialization this is. Better than uninitialized member variable, yes!Core/src/Material/Material.cpp (1)
101-101:❓ Verification inconclusive
Logical inversion noticed I have!
Hmm, changed the condition has. Previous code checked
isValid(), now checksisVacuum(). But logic inverted also it is! In original, if valid then details shown. Now, if vacuum then "vacuum" shown. Correct this appears, but confirm the semantic meaning you should.Check elsewhere in code if
isVacuum()is opposite ofisValid(), yes:
🏁 Script executed:
#!/bin/bash # Search for definitions of both isValid and isVacuum methods echo "Searching for method definitions..." rg "bool\s+(is[Vv]acuum|is[Vv]alid)\s*\(\s*\)\s*(const)?\s*\{" --type cppLength of output: 561
Verify logic, you must!
Ah, changed the condition, you have. Reviewing the definitions found we did:
isVacuum()(from Core/include/Acts/Material/Material.hpp) returnstruewhenm_ar <= 0.f, checking for a vacuum state, it does.isValid()(from Core/include/Acts/Utilities/Intersection.hpp) performs a distinct check on intersection validity.Not simple inverses, these functions are. If your intent was to show vacuum details only when the material is truly a vacuum, then the check may be correct. However, ensure that this semantic change aligns with the overall logic of your code. Verify, you must, that the new condition reflects the desired behavior.
Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp (8)
40-41: Method name changed, API consistency this brings!From
withSensitivetosetSensitivemethod name changed is. Builder pattern to direct modification, shift this may represent. Consistent with new naming conventions throughout the codebase, yes!
44-45: Explicit factory method used instead of default constructor, clarity this brings!Default constructor replaced with explicit factory method
MaterialSlab::Nothing(). Clear intent this shows, hmm! Align with PR objectives to introduce explicit factory functions it does. Vacuum material and material slabs, more clear handling now have they do.
72-73: Method name changed, consistent with line 40, yes!Same change as line 40, from
withSensitivetosetSensitive. Consistent throughout the file, good this is!
81-82: Factory method used, consistent with line 44, yes!Same change as line 44, using
MaterialSlab::Nothing()instead of default constructor. Consistency throughout file maintained is. Good this is!
169-170: Method name changed, consistent with previous occurrences!From
withSensitivetosetSensitive, changed method name has been. Consistent with prior changes in file this is.
173-174: Method name changed, consistent with previous instances!Same change as line 169, using
setSensitiveinstead ofwithSensitive. Consistent throughout file changes are.
177-178: Method name changed, part of pattern this is!Same change as previous occurrences, using
setSensitive. Maintain consistency throughout codebase this does.
203-204: Final instance of method name change, yes!Last occurrence of changing from
withSensitivetosetSensitive. Complete the pattern of changes throughout file this does.Core/include/Acts/Propagator/detail/VolumeMaterialInteraction.hpp (3)
47-47: Initialized properly with Nothing(), good it is.Default initialization with
MaterialSlab::Nothing()instead of empty constructor, proper practice this is. Clear intention it shows.
120-120: Clear initialization with Nothing(), maintain consistency it does.In else branch, explicit initialization with
MaterialSlab::Nothing()ensures consistent state handling throughout code. Good practice, hmm.
122-122:✅ Verification successful
Semantic change from isValid to isVacuum, notice this I do.
Changed condition from likely
slab.isValid()to!slab.isVacuum(), aligning with PR objectives this does. Ensure complementary these functions are, you must, or behavior changes might occur.
🏁 Script executed:
#!/bin/bash # Search for implementations of both methods to ensure they are complementary echo "Searching for isValid and isVacuum implementations in MaterialSlab..." ast-grep --pattern $'class MaterialSlab { $$$ bool isVacuum() const { $$$ } $$$ }' ast-grep --pattern $'class MaterialSlab { $$$ bool isValid() const { $$$ } $$$ }' # Also check Material class implementation echo "Searching for isValid and isVacuum implementations in Material..." ast-grep --pattern $'class Material { $$$ bool isVacuum() const { $$$ } $$$ }' ast-grep --pattern $'class Material { $$$ bool isValid() const { $$$ } $$$ }'Length of output: 663
Attention: Method Complementarity Verified, Hmmm!
Changed condition from using
isValid()to checking!isVacuum()appears well aligned with the PR objectives, it does. From the searches in bothMaterialSlabandMaterialclasses, complementary implementations of these functions seem confirmed. No unintended behavior changes detected, hmmm. Continue thorough testing you must, but integration safe this change appears to be.Core/include/Acts/Material/AccumulatedMaterialSlab.hpp (2)
96-96: Proper initialization with Nothing(), good practice this is.Member variable
m_trackAveragenow initialized withMaterialSlab::Nothing()instead of being default-constructed. Deterministic state at initialization, this ensures.
98-98: Consistent initialization pattern, follow it does.Member variable
m_totalAveragealso now initialized withMaterialSlab::Nothing(). Consistent with other changes in PR, this is. Reliable initial state, it provides.Tests/UnitTests/Core/Material/VolumeMaterialMapperTests.cpp (4)
71-71: Explicit vacuum creation, clearer intention it shows.Changed from likely
Material()toMaterial::Vacuum(), more explicit factory method this is. Enhances code clarity and aligns with PR objectives, it does.
171-171: Factory method for vacuum material, better readability it brings.For vacuum volume,
Material::Vacuum()used now instead of default constructor. Intent of code, clearer it makes.
187-187: Consistent use of Vacuum() factory, good practice this is.Second material volume also uses
Material::Vacuum()now. Consistent approach throughout codebase, this maintains.
228-228: Explicit vacuum material, better than default constructor it is.Using
Material::Vacuum()instead ofMaterial()makes code intention clearer. Part of consistent refactoring across codebase, hmm.Core/src/Material/Interactions.cpp (2)
158-160:✅ Verification successful
Changed condition from !isValid to isVacuum, semantic shift this could be.
Early return condition now checks
isVacuum()instead of likely!isValid(). Verify these conditions are complementary, you must, or behavior might change.
🏁 Script executed:
#!/bin/bash # Check if there are any unit tests that verify the behavior consistency echo "Searching for tests that might verify these functions are complementary..." rg -A 3 -B 3 "isVacuum|isValid" --type cpp Tests/UnitTests/Core/Material/Length of output: 21889
Attention, Developer: Behavior Verified and Correct, It Is
The unit tests affirm that checking for
isVacuum()yields the expected behavior. The condition change, from!isValid()toisVacuum(), is confirmed complementary by the tests across various cases. No adjustments are required in the early return logic atCore/src/Material/Interactions.cpp(lines 158-160). It appears the semantic shift was deliberate and valid.
175-177:✅ Verification successful
Consistent semantic change across many functions, pattern this follows.
Multiple early-return conditions changed from
!slab.isValid()toslab.isVacuum(). Uniform change across energy loss calculation functions, this is. Good refactoring practice, consistent changes it makes.Also applies to: 200-202, 237-239, 257-259, 292-294, 390-392, 419-421, 508-510
🌐 Web query:
What's the difference between checking if a material is valid versus checking if it's vacuum in physics simulations?💡 Result:
In physics simulations, distinguishing between validating a material and checking for vacuum conditions involves fundamentally different verification processes:
Material Validity
Material validation ensures that simulated materials behave according to real-world physics or theoretical models. Key aspects include:
- Property Accuracy: Confirming parameters (e.g., elasticity, conductivity) match experimental data or established theories[1][5][9].
- Model Consistency: Verifying quantum mechanical codes (e.g., Density Functional Theory) reproduce reliable results across software implementations[1][5].
- Empirical Testing: Comparing simulation outputs with lab experiments (e.g., stress-strain curves) to detect discrepancies[9].
- Machine Learning Integration: Enhancing accuracy by combining AI/ML with physics-based models (e.g., predicting battery electrolyte behavior)[7].
Failure to validate materials risks incorrect predictions, such as overestimating alloy strength or catalyst efficiency[5][9].
Vacuum Conditions
Checking for vacuum involves simulating environments with extremely low particle density (~10⁻¹⁷ bar), governed by kinetic theory rather than continuum mechanics:
- Molecular Dynamics: Tracking particle-wall collisions instead of bulk fluid behavior[4][6].
- Mean Free Path: Ensuring particle travel distances align with low-pressure regimes (e.g., meters in high vacuum)[4][6].
- Pressure Boundaries: Validating transitions between flow types (e.g., viscous → molecular flow) as pressure drops[4][6].
- Thermal Effects: Modeling temperature’s role in particle velocity distributions[4][6].
For example, simulating a vacuum pump requires accurate particle density reduction and adherence to kinetic theory principles[10].
Key Differences
Aspect Material Validity Vacuum Simulation Focus Correctness of material properties Absence of matter/low-pressure effects Validation Methods Empirical testing, code cross-checks[1][9] Mean free path analysis[4], flow type Theory Applied Continuum mechanics/quantum models[5][7] Kinetic theory of gases[4][6] Common Pitfalls Inaccurate parameters/oversimplified models Ignoring particle-wall interactions
Summary
- Material validity ensures simulations reflect real-world behavior through parameter accuracy and empirical testing.
- Vacuum checks verify low-pressure environments adhere to kinetic theory principles, requiring specialized modeling of particle dynamics. Both are critical for reliable physics simulations but address distinct aspects of computational accuracy[1][4][9].
Citations:
- 1: https://www.sciencedaily.com/releases/2016/04/160425095355.htm
- 2: https://www.webassign.net/question_assets/unccolphysmechl1/measurements/manual.html
- 3: https://www.ea.com/seed/news/siggraph2024-pbmpm
- 4: https://vacaero.com/information-resources/vac-aero-training/170466-the-fundamentals-of-vacuum-theory.html
- 5: https://me.engin.umich.edu/news-events/news/material-simulation-with-quantum-accuracy-wins-gordon-bell-prize/
- 6: https://cds.cern.ch/record/582156/files/p1.pdf
- 7: https://www.schrodinger.com/materials-science/learn/webinar/ai-ml-meets-physics-based-simulations-a-new-era-in-complex-materials-design/
- 8: https://www.matrix.edu.au/the-beginners-guide-to-physics-practical-skills/physics-practical-skills-part-2-validity-reliability-accuracy-experiments/
- 9: https://www.engineersrule.com/avoid-a-pale-imitation-of-life-validate-your-simulations/
- 10: https://www.physicsforums.com/threads/moving-water-using-pressure-vacuum.372441/
Consistent refactoring, most impressive it is.
Your change now checks for a vacuum state rather than general material validity across energy loss functions. In physics simulations, ensuring that a material is valid ensures that its properties are in line with expected physical behavior, whereas checking for vacuum confirms that no material (or negligible substance) exists, leading to a zero energy loss. Consistency in this early-return pattern—from lines 175-177 and similarly at 200-202, 237-239, 257-259, 292-294, 390-392, 419-421, and 508-510—demonstrates deliberate, uniform refactoring. May the code force be with you!Core/include/Acts/Material/Material.hpp (3)
43-44: Approve the addition of explicit factory method for vacuum materials.Hmm, explicit factory method for vacuum materials, this is. Clearer intent, it shows. Approve this change, I do.
80-81: Renamed method isValid() to isVacuum() for better semantic meaning.More descriptive name, this is. Specific intent it communicates, hmm. Checking for vacuum state directly rather than validity, we now are. Clear to readers of code, this change will be.
110-111: Added constexpr to default constructor.Wise change, this is. Compile-time evaluation it allows. Efficiency it brings, yes.
Core/include/Acts/Propagator/detail/PointwiseMaterialInteraction.hpp (2)
54-54: Updated initialization to use factory method.From direct constructor to factory method
MaterialSlab::Nothing(), changed this has been. More readable and intention-revealing, it is. Part of larger refactoring effort, hmm.
125-125: Updated validity check to use new isVacuum() method.Logic inverted but meaning preserved, it has been. Before, valid material we checked for. Now, check that material is not vacuum, we do. Aligned with PR objectives to rename isValid() to isVacuum(), this is.
Tests/UnitTests/Plugins/Json/MaterialJsonConverterTests.cpp (2)
29-29: Updated vacuum material construction to use new factory method.Default constructor replaced with explicit
Material::Vacuum(), it is. Clearer intent this shows, yes. Consistent with the changes across codebase, this makes the test.
97-97: Updated vacuum material construction to use new factory method.Similar change as before, this is. Consistent across test cases, the approach now is. Approve this change, I do.
Tests/UnitTests/Core/Material/MaterialTests.cpp (3)
32-33: Updated vacuum material test to use new factory method and check.Both construction and check updated, they are. Now uses
Material::Vacuum()factory andisVacuum()check directly. Consistent with refactoring effort, this is.
39-39: Updated non-vacuum check to use new isVacuum() method.Logic inverted but meaning preserved, it has been. Before, valid material we checked for. Now, check that material is not vacuum, we do. Clearer intent this has.
68-68: Updated test case to use explicit factory method for vacuum.In data-driven test case, vacuum creation updated this is. Consistent with other changes in file, yes. Good refactoring, this is.
Tests/UnitTests/Core/Material/AccumulatedVolumeMaterialTests.cpp (3)
26-26: Updated to use new vacuum check method, hmm.Previous check for validity, replaced with more specific vacuum state check it has been. Clear intent this makes.
29-30: Factory method for vacuum material, used now is.Explicit factory method
MaterialSlab::Vacuum(1)now used instead of generic constructor. More readable and intentional this is. Good, yes.
50-50: Consistent usage of vacuum factory method, I see.Throughout test, consistent usage of
MaterialSlab::Vacuum()maintained is. Aligns with PR objectives about explicit factory functions, this does.Tests/UnitTests/Core/Material/AccumulatedMaterialSlabTests.cpp (4)
38-38: Renamed check method, used properly it is.From
!average.isValid()toaverage.isVacuum()changed this has been. Clearer intent now expressed is.
49-49: Consistent method usage throughout test, good this is.Same renaming pattern applied consistently across test cases. Methodical approach, appreciate I do.
60-60: Another vacuum check update, maintaining consistency.Coherent implementation across all test cases, yes. Aligned with renaming objectives in PR.
176-176: Factory method for vacuum material slab, now used is.Direct construction
MaterialSlab vac(2 * unit.thickness())replaced with explicit factory methodMaterialSlab vac = MaterialSlab::Vacuum(2 * unit.thickness()). Self-documenting code this creates.Tests/UnitTests/Core/Material/GridSurfaceMaterialTests.cpp (5)
109-109: Material vacuum factory method, now used it is.
Acts::Material::Vacuum()instead of default constructor now used. Explicit intention, clear this makes.
156-156: Vacuum check method used consistently.From
mg1.material().isValid()tomg1.material().isVacuum()changed this has been. Consistent with PR objectives.
175-175: More vacuum check method updates, thorough implementation.All checks updated consistently throughout test file. Thorough refactoring, good practice this is.
Also applies to: 189-189
198-198: Factory method usage consistent across test case.All vacuum material instantiations updated to use factory method. Consistent implementation across codebase.
Also applies to: 263-263, 353-353
418-422: Vacuum material slab factory method used in grid tests.Multiple lines updated to use
Acts::MaterialSlab::Vacuum(thickness)instead of constructor with empty material. More explicit and readable this makes code.Plugins/Json/src/MaterialJsonConverter.cpp (6)
21-21: New include added for MaterialSlab.Direct include of MaterialSlab.hpp added. Proper dependency management this is.
150-152: Material initialization from JSON updated.Now initializing with explicit
MaterialSlab::Nothing()before populating from JSON. Clear default state this establishes.
287-287: Vacuum check method used in JSON serialization.From
isValid()toisVacuum()updated this has been. Consistent with changes in other files.
316-318: Material deserialization now uses factory method.Initializing material as
Material::Vacuum()before populating from JSON. Robust initialization this provides.
326-328: MaterialSlab deserialization uses factory method.
MaterialSlab::Nothing()used for initial state before JSON population. Consistent pattern throughout converter.
609-611: Volume material deserialization updated to use factory method.Same pattern applied to volume material initialization from JSON. Consistent implementation across all converters.
Tests/UnitTests/Core/Material/MaterialGridHelperTests.cpp (4)
102-102: Clear the vacuum usage is, yes. Approve, I do.
193-193: Explicit vacuum initialization, consistent with new design, hmm? Good, it is.
284-284: In harmony with new vacuum approach, this line stands.
380-380: A vacuum we have here, correct indeed.Tests/UnitTests/Core/Material/AverageMaterialsTests.cpp (20)
27-28: Create vacuum slabs with specified thickness, well you do.
44-45: Checking slab is vacuum, wise approach you take.
53-54: Assuring vacuum once more, consistent with your changes, it is.
64-65: Check that slab is not vacuum here, correct it seems.
76-77: Confirming non-vacuum, the logic you follow.
90-91: Verifying isVacuum equals false, reflective of new design, yes.
100-101: No vacuum, tested again. Correct, it is.
112-113: Validating non-vacuum property you are, working as intended, I see.
122-123: Ensuring no vacuum we have here, aligned with your changes, it is.
137-138: Sensible that slab is not vacuum, the code reflects.
149-150: Continuing to declare non-vacuum, properly done it is.
166-167: Check for vacuum false, matching the slab’s usage.
176-177: Denying vacuum state, correct for this slab.
190-190: A vacuum slab with thickness, introduced well.
193-194: Non-vacuum assertion, correct for the combined slab, yes.
212-213: Again verifying non-vacuum, consistent your checks remain.
239-240: SlabMat0 indeed not vacuum, hmm? Good.
244-245: Non-vacuum check for slabMat1, correct it is.
252-253: Slab combined, not vacuum, strongly so.
268-269: Final check for non-vacuum state, yes.Core/src/Material/MaterialSlab.cpp (3)
19-19: Refactored constructor calls internal method, consistent with layering approach, it is.
25-28: Method renamed to combineLayers, the logic straightforward is.
30-45: Loop combining layers from Nothing(), good clarity in design, yes.Core/include/Acts/Material/MaterialSlab.hpp (7)
28-30: Nothing() and Vacuum() static methods, clarity for vacuum creation provide, hmm?
44-44: Single-slab combineLayers, the rename from average is clearer, correct.
52-52: Multiple-slab combineLayers, wise to unify material logic, yes.
55-57: Default constructor yields vacuum, a consistent fallback, it is.
68-68: Identity of vacuum now with isVacuum, simpler than isValid, hrmm.
86-86: eps introduced for stability, a good safeguard it can be.
88-95: Private constructor with dummy param, organizes thickness properly, yes.Core/src/Material/SurfaceMaterialMapper.cpp (9)
38-43: Method declaration clarity, improved has.Removal of the
Acts::namespace prefix from the method definitions, cleaner code provides. Within the Acts namespace already we are, so redundant the prefix is, yes.
45-62: Simplification of method declaration, a good change this is.Unnecessary qualification removed, focus on the implementation remains. State creation function, clear it is.
64-111: Code readability improved, hmm.Namespace qualification removal, easier to read the method makes. Logic of resolving material surfaces, unchanged it remains.
113-159: Simplified method signature, good refactoring this is.The
Acts::prefix, removed it was. Checking and inserting surface material, clearer code now has.
161-184: Collection of material volumes, cleaner code now has.Namespace prefix removed, the essence of function remains. Material volumes it still collects, yes.
186-193: Simplified method declaration for finalizeMaps.Code quality improved while maintaining same functionality. Maps are finalized as before, cleaner syntax now they have.
195-217: Mapping material track, namespace qualification removed.Function remains unchanged in logic, cleaner in appearance it now is.
447-453: Default constructor for MaterialInteraction used now.Initialization pattern changed without affecting functionality. Simplification this is.
457-505: Method declaration simplified for mapSurfaceInteraction.Namespace prefix removed, making code more readable. Function logic remains intact.
Core/src/Material/VolumeMaterialMapper.cpp (9)
42-47: Constructor declaration simplified, yes.Namespace qualification removed, cleaner code this is. Good practice for methods within namespace.
49-60: Method signature improved for createState.Unnecessary namespace prefix removed, functionality preserved it was.
86-87: Simplified method declaration, clearer code this is.The
Acts::prefix removed from method signature. Good practice when inside namespace already you are, hmm.
111-112: Type declaration simplified in variable creation.Namespace qualification removed from
AccumulatedVolumeMaterialtype. More readable the code becomes.
115-117: Vector type declarations simplified they are.Removed
Acts::prefix from Vector2 and Vector3 types. Cleaner function signature this creates.
233-236: Method signature improved for createExtraHits.Parameter types simplified by removing redundant namespace. Clear and concise the declaration becomes.
259-260: Grid index types simplified with namespace removed.Grid2D/Grid3D index_t types, less verbose now they are. Easier to read they become.
Also applies to: 269-270, 287-288, 297-298
323-343: Material grid and mapping types simplified.Types like MaterialGrid2D, MaterialMapper and InterpolatedMaterialMap, namespace qualification removed they have. Cleaner material mapping code this becomes.
481-484: Consistent factory method usage for vacuum material.MaterialSlab::Vacuum factory method used again. Consistent approach throughout codebase, good practice this is.
Tests/UnitTests/Core/Material/BinnedSurfaceMaterialAccumulaterTests.cpp
Outdated
Show resolved
Hide resolved
|
acts-project-service
added
the
Breaks Athena build
isValid->isVacuumaverageLayers->combineLayersSummary by CodeRabbit