Update ROOTFrameWriter.cc #494
Conversation
There was a problem hiding this comment.
Hi @whitehawk0910,
thanks for taking a stab at this. It looks like there are some changes in include/podio/ROOTFrameWriter.h that are not committed (or at least not yet part of this PR.). This (and a few other things, see below) currently makes it impossible to build this. I am not sure you actually need to introduce new member functions after addressing the comments below, so this might no longer be true in another iteration of this PR.
As a general comment: If someone tries to write an inconsistent Frame we would like more than a printed warning. This should raise an exception.
Finally, it looks like you (or your editor) formatted this using the wrong settings. We currently use clang-format version 16 to format all our code via clang-format --style=file.
| if (!catInfo.collInfo.empty()) { | ||
| // If not the first frame, compare the contents with the stored contents | ||
| const auto& storedContents = getStoredCategoryContents(category); | ||
| if (frame.contents() != storedContents) { |
There was a problem hiding this comment.
The Frame doesn't have a contents method. Instead there is a getAvailableCollections that should give you what you want.
| // Check if this is the first frame of the category | ||
| if (!catInfo.collInfo.empty()) { | ||
| // If not the first frame, compare the contents with the stored contents | ||
| const auto& storedContents = getStoredCategoryContents(category); |
There was a problem hiding this comment.
At this point you do not really need this indirection. You already have the category information in catInfo and you can simply do catInfo.collsToWrite to achieve the same thing. The lookup in getStoredCategoryContents is superfluous.
| if (!catInfo.collInfo.empty()) { | ||
| // If not the first frame, compare the contents with the stored contents | ||
| const auto& storedContents = getStoredCategoryContents(category); | ||
| if (frame.contents() != storedContents) { |
There was a problem hiding this comment.
Unless you have explicitly implemented this operator!= overload to compare two std::vector<std::string> this will not compile.
|
Superseded by #513 |
Pull Request Summary
Description:
This pull request introduces enhancements to the ROOTFrameWriter class in the Podio library. It implements a warning system for inconsistent contents within frames of the same category. Additionally, it integrates a DatamodelDefinitionCollector to register datamodel definitions for collections.
Issues Resolved:
This pull request resolves issue #381 and aims to improve the robustness and consistency of data handling in the library.
Changes Made
Modification in ROOTFrameWriter.h:
Added a warning system for inconsistent content.
Implemented a check for consistent content across frames in the same category.
Modification in ROOTFrameWriter.cpp:
Integrated the warning system within the writeFrame function.
Added a helper function for issuing warnings and obtaining stored category contents.
Additional Changes:
Improved comments for clarity and understanding.
Integrated a DatamodelDefinitionCollector to register datamodel definitions.
Testing
The changes have been tested as follows:
Built the code successfully.
Ran all existing tests, and they passed.
Added new tests to cover the changes, specifically testing the warning system and datamodel registration.
Verified that the documentation is up to date.
Known Issues
No known issues or concerns at the moment.
Checklist
The code builds successfully.
All existing tests pass.
New tests have been added to cover the changes.
Documentation has been updated to reflect the changes.
Additional Notes
These changes improve the reliability of the ROOTFrameWriter and enhance its ability to handle data consistency.
BEGINRELEASENOTES
exactly as it appears between the two bold lines
ENDRELEASENOTES