preparing ground for electromag diagnostics for mhd #999
Conversation
📝 WalkthroughWalkthroughThe changes refactor how electromagnetic diagnostic fields are handled by replacing generic iteration over a collection of fields with explicit handling of the "EM_B" and "EM_E" fields. A new helper method is introduced to check if a diagnostic is active for a given field. The model view interface is updated by removing a method that returns all electromagnetic fields and adding separate accessors for the B and E fields. All relevant logic in the diagnostic writer is updated to use these explicit accessors and conditional checks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ElectromagDiagnosticWriter
participant ModelView
participant VecField_B
participant VecField_E
User->>ElectromagDiagnosticWriter: Request diagnostic operation
ElectromagDiagnosticWriter->>ElectromagDiagnosticWriter: isActiveDiag("EM_B")
alt if "EM_B" active
ElectromagDiagnosticWriter->>ModelView: getB()
ModelView-->>ElectromagDiagnosticWriter: VecField_B
ElectromagDiagnosticWriter->>VecField_B: Process B field
end
ElectromagDiagnosticWriter->>ElectromagDiagnosticWriter: isActiveDiag("EM_E")
alt if "EM_E" active
ElectromagDiagnosticWriter->>ModelView: getE()
ModelView-->>ElectromagDiagnosticWriter: VecField_E
ElectromagDiagnosticWriter->>VecField_E: Process E field
end
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 1
🧹 Nitpick comments (3)
src/diagnostic/detail/types/electromag.hpp (3)
52-56: Simplify isActiveDiag implementationThe isActiveDiag method is a good refactoring that extracts a common check, but the parameter naming could be improved for clarity.
Consider this improved implementation:
- auto isActiveDiag(DiagnosticProperties const& diagnostic, std::string const& tree, - std::string var) + auto isActiveDiag(DiagnosticProperties const& diagnostic, std::string const& prefix, + std::string const& fieldName) { - return diagnostic.quantity == tree + var; + return diagnostic.quantity == prefix + fieldName; };This clarifies the purpose of each parameter and makes the code more self-documenting.
61-65: Simplify createFiles methodThe current implementation creates a local variable
treethat's only used once. This could be simplified further.void ElectromagDiagnosticWriter<H5Writer>::createFiles(DiagnosticProperties& diagnostic) { - std::string tree = "/"; - checkCreateFileFor_(diagnostic, fileData_, tree, "EM_B", "EM_E"); + checkCreateFileFor_(diagnostic, fileData_, "/", "EM_B", "EM_E"); }
168-180: Refine the use of tree variableThe
treevariable is created but only used within theisActiveDiagcalls. This pattern is repeated in several methods and could be simplified.- std::string tree = "/"; std::string path = h5Writer.patchPath() + "/"; - if (isActiveDiag(diagnostic, tree, "EM_B")) + if (isActiveDiag(diagnostic, "/", "EM_B")) { auto& B = h5Writer.modelView().getB(); h5Writer.writeTensorFieldAsDataset(h5file, path + "EM_B", B); } - if (isActiveDiag(diagnostic, tree, "EM_E")) + if (isActiveDiag(diagnostic, "/", "EM_E")) { auto& E = h5Writer.modelView().getE(); h5Writer.writeTensorFieldAsDataset(h5file, path + "EM_E", E); }Alternatively, if this prefix is likely to change or be used in multiple places, consider defining it as a class constant.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/diagnostic/detail/types/electromag.hpp(5 hunks)src/diagnostic/diagnostic_model_view.hpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.hpp`: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
src/diagnostic/diagnostic_model_view.hppsrc/diagnostic/detail/types/electromag.hpp
🧬 Code Graph Analysis (1)
src/diagnostic/diagnostic_model_view.hpp (2)
src/core/data/ions/ion_population/ion_population.hpp (2)
VecField(86-86)VecField(87-87)src/core/data/electrons/electrons.hpp (5)
VecField(91-102)VecField(274-274)VecField(276-276)VecField(350-350)VecField(355-355)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
src/diagnostic/detail/types/electromag.hpp (2)
93-102: LGTM! Good refactoring of field accessThe refactoring to check for specific diagnostic field types and retrieve them individually through dedicated accessors makes the code more explicit and easier to understand.
143-154: LGTM! Consistent pattern for field initializationThe code consistently applies the same pattern of checking if a diagnostic is active for a specific field and then initializing it. This makes the control flow more explicit and easier to follow.
| NO_DISCARD VecField& getB() const { return model_.state.electromag.B; } | ||
|
|
||
| NO_DISCARD VecField& getE() const { return model_.state.electromag.E; } |
There was a problem hiding this comment.
Fix const-correctness issues in getB() and getE() methods
The current implementation of both methods violates const-correctness principles by returning non-const references from const methods. This can lead to unexpected behavior where supposedly immutable objects can be modified.
Apply this diff to fix the const-correctness:
- NO_DISCARD VecField& getB() const { return model_.state.electromag.B; }
+ NO_DISCARD VecField const& getB() const { return model_.state.electromag.B; }
+ NO_DISCARD VecField& getB() { return model_.state.electromag.B; }
- NO_DISCARD VecField& getE() const { return model_.state.electromag.E; }
+ NO_DISCARD VecField const& getE() const { return model_.state.electromag.E; }
+ NO_DISCARD VecField& getE() { return model_.state.electromag.E; }This follows the pattern seen elsewhere in the codebase for accessor methods, providing both const and non-const versions that return const and non-const references respectively.
📝 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.
| NO_DISCARD VecField& getB() const { return model_.state.electromag.B; } | |
| NO_DISCARD VecField& getE() const { return model_.state.electromag.E; } | |
| NO_DISCARD VecField const& getB() const { return model_.state.electromag.B; } | |
| NO_DISCARD VecField& getB() { return model_.state.electromag.B; } | |
| NO_DISCARD VecField const& getE() const { return model_.state.electromag.E; } | |
| NO_DISCARD VecField& getE() { return model_.state.electromag.E; } |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| if (isActiveDiag(diagnostic, tree, "EM_B")) | ||
| { | ||
| auto& B = h5Writer.modelView().getB(); |
Check notice
Code scanning / CodeQL
Unused local variable Note
| auto& name = vecField->name(); | ||
| if (diagnostic.quantity == "/" + name) | ||
| initVF(path, attr, name, null); | ||
| auto& E = h5Writer.modelView().getE(); |
Check notice
Code scanning / CodeQL
Unused local variable Note
| }; | ||
|
|
||
|
|
||
| template<typename H5Writer> | ||
| void ElectromagDiagnosticWriter<H5Writer>::createFiles(DiagnosticProperties& diagnostic) | ||
| { | ||
| for (auto* vecField : this->h5Writer_.modelView().getElectromagFields()) | ||
| checkCreateFileFor_(diagnostic, fileData_, "/", vecField->name()); | ||
| std::string tree = "/"; |
| h5Writer.writeTensorFieldAsDataset(*fileData_.at(diagnostic.quantity), | ||
| h5Writer.patchPath() + "/" + vecField->name(), | ||
| *vecField); | ||
| std::string tree = "/"; |
Changed ElectromagDiagnosticWriter to not rely on the vecfield names as keys to work with strings from python. Also implemented separate getters for E and B, which were previously given in a single vector for iteration purposes.