Add `setCreationMode` to `FairParSet` by karabowi · Pull Request #1552 · FairRootGroup/FairRoot

karabowi · 2024-06-06T08:52:18Z

In rare cases, when parameteres deriving from FairParSet
are not present in the input parameter file and are meant
to be created during the run, FairParSet::init() throws
an error message.

By using the newly introduced setter setCreationMode
this message is suppressed.

This allows solution to issue #1541.

The PR also does some clean-up in FairParSet.

Checklist:

Followed the Contributing Guidelines

Summary by CodeRabbit

New Features
- Introduced a new setCreateMode() method to manage creation modes.
Deprecations
- Deprecated unused methods fill() and store() in the FairParSet class.
Improvements
- Updated data types for better consistency and performance.
- Enhanced logging for improved debugging and maintenance.
Documentation
- Provided guidance on deprecated APIs in the changelog.

In rare cases, when parameteres deriving from `FairParSet` are not present in the input parameter file and are meant to be created during the run, `FairParSet::init()` throws an error message. By using the newly introduced setter `setCreationMode` this message is suppressed. This allows solution to issue FairRootGroup#1541.

…Set` These functions seem not be used in `FairRoot`. Deprecated them.

coderabbitai · 2024-06-06T08:52:26Z

Walkthrough

The 19.0.1 update for FairRoot focuses on modernizing the FairParSet class by deprecating unused methods, introducing a new setCreateMode() method, and updating data types from Int_t and Bool_t to int and bool for clarity and consistency. Additionally, logging improvements and code cleanups were made to enhance readability and maintainability.

Changes

File	Change Summary
`CHANGELOG.md`	Documented deprecation of unused methods, introduction of `setCreateMode()`, and guidance on deprecated APIs.
`fairroot/parbase/FairParSet.cxx`	Added `creationMode` variable, updated data types, improved logging, and replaced `kFALSE`/`kTRUE` with `false`/`true`.
`fairroot/parbase/FairParSet.h`	Updated data types, added `creationMode` variable, introduced new methods, and increased `ClassDefOverride` version.

Sequence Diagram(s) (Beta)

sequenceDiagram
    participant User
    participant FairParSet
    User ->> FairParSet: setCreateMode(true)
    FairParSet ->> FairParSet: creationMode = true
    User ->> FairParSet: isCreationMode()
    FairParSet -->> User: true

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6f693ea and 51c73a4.

Files selected for processing (3)

CHANGELOG.md (2 hunks)
fairroot/parbase/FairParSet.cxx (5 hunks)
fairroot/parbase/FairParSet.h (4 hunks)

Additional context used

LanguageTool

CHANGELOG.md

[style] ~66-~66: ‘was in conflict with’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_WAS_IN_CONFLICT_WITH)
Context: ...pped build switch BUILD_UNITTESTS, it was in conflict with the CMake standard switch [`BUILD_T...

[uncategorized] ~115-~115: A period might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
Context: ...` isn't searched for and not needed any more ### Example Changes in Experiment Repo...

[uncategorized] ~152-~152: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ... Stop run if false returned. * Remove sink from Tutorial3/MQ/sampler.cxx. * Fixed ...

[uncategorized] ~156-~156: You might be missing the article “an” here. (AI_EN_LECTOR_MISSING_DETERMINER_AN)
Context: ...Notable Changes * Allow running without output sink. In this case even persistent bran...

[formatting] ~156-~156: Consider inserting a comma after an introductory phrase for better readability. (IN_THAT_CASE_COMMA)
Context: ...es * Allow running without output sink. In this case even persistent branches would not be s...

[uncategorized] ~164-~164: A comma may be missing after the conjunctive/linking adverb ‘Also’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ... the Base into the Online library. * Also the MBS/LMD related source classes were...

[uncategorized] ~166-~166: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...ne library. * If one of these classes are used in your code, you need to link...

[style] ~184-~184: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ... being set, rewrite it accordingly. * If your code has specific C++ needs, consi...

[misspelling] ~187-~187: This word is normally spelled as one. (EN_COMPOUNDS_ANTI_PATTERN)
Context: ...USE_PATH_INFO CMake option. It was an anti-pattern. Don't use it! * Dropped `ROOT_GENERATE...

[uncategorized] ~196-~196: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION)
Context: ... You might have to add some #includess in your code. * Added a new build switch...

[uncategorized] ~200-~200: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...ll now require its external package dependencies FairMQ and Boost. * Boost serialization...

[grammar] ~203-~203: The word “opt-in” is a noun. The verb is spelled with a space. (NOUN_VERB_CONFUSION)
Context: ...on Boost.Serialization. The user must opt-in to the previously baseline Boost serial...

[grammar] ~203-~203: You used an adverb (‘previously’) instead of an adjective, or a noun (‘baseline’) instead of another adjective. (A_RB_NN)
Context: ...ialization. The user must opt-in to the previously baseline Boost serialization support by includ...

[typographical] ~216-~216: The conjunction “so that” does not have a comma in front. (SO_THAT_UNNECESSARY_COMMA)
Context: ...y require some API, please file an issue, so that we can see how to handle this. * Depre...

[uncategorized] ~228-~228: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...tion. * FairRadMapManager::Instance, FairRadLenManager::Instance *Fair...

[uncategorized] ~232-~232: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...thing. * FairTask::*InputPersistance: There's no getter for `fInputPersis...

[uncategorized] ~239-~239: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ConstructASCIIGeometry(T*, TString, U*), use FairModule::ConstructASCIIGeom...

[uncategorized] ~246-~246: This verb may not be in the correct tense. Consider changing the tense to fit the context better. (AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
Context: ...rs should you need it. * Scheduled to remove without replacement in a future major r...

[uncategorized] ~265-~265: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...se` style naming. For those using the targets this change will not be visible. * Test...

[formatting] ~266-~266: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause. (COMMA_BEFORE_BECAUSE)
Context: ...ing Geant3 have been disabled by default, because those tests have a probability > 0 fo...

[style] ~268-~268: The word ‘anyways’ is informal American English. Did you mean “anyway”? (ANYWAYS)
Context: ... for failing. If you want to run them anyways, pass -DENABLE_GEANT3_TESTING=ON to...

Markdownlint

CHANGELOG.md

123-123: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

135-135: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

143-143: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

149-149: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

155-155: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

160-160: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

248-248: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines

44-44: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

47-47: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

171-171: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

173-173: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

174-174: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

176-176: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

177-177: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

181-181: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

205-205: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

208-208: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines

124-124: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines

136-136: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines

144-144: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines

150-150: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines

156-156: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines

161-161: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines

249-249: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines

28-28: null (MD034, no-bare-urls)
Bare URL used

30-30: null (MD034, no-bare-urls)
Bare URL used

100-100: null (MD034, no-bare-urls)
Bare URL used

119-119: null (MD034, no-bare-urls)
Bare URL used

Additional comments not posted (10)

fairroot/parbase/FairParSet.h (5)

23-27: Update to use standard C++ types and addition of creationMode variable.

This change aligns with modern C++ standards and adds a new feature for managing creation mode, which is a good practice for enhancing functionality without breaking existing interfaces.

33-33: Constructor updated to use standard C++ boolean type.

Switching from Bool_t to bool is a positive change towards standardizing the codebase and reducing dependency on ROOT-specific types.

49-55: Updated method signatures to use standard C++ types.

The consistent use of standard types (int and bool) instead of ROOT types (Int_t, Bool_t) across these methods improves the code's portability and readability.

Also applies to: 64-65, 67-68, 70-71, 73-74

92-93: Deprecation of unused methods fill and store.

Marking unused methods as deprecated is a good practice. It warns other developers about the obsolescence of these methods, potentially guiding them towards newer or more efficient alternatives.

98-98: Class version updated in ClassDefOverride.

Incrementing the class version after making significant changes is necessary for maintaining compatibility with ROOT's I/O functionality.

fairroot/parbase/FairParSet.cxx (4)

34-39: Initialization of new member creationMode and default values for versions.

Proper initialization of class members in the constructor prevents undefined behavior and ensures that the object starts in a valid state.

Line range hint 54-77: Enhanced error handling in init method with respect to creationMode.

The conditional handling based on creationMode allows the system to operate in different configurations, which is useful for suppressing unnecessary error messages during initialization.

99-112: Use of logging instead of direct output to cout.

Replacing cout with logging statements is a best practice for production code, as it allows better control over the output and integrates with system-wide logging policies.

117-128: Resetting input versions and status in clear method.

Clearing or resetting the state of an object in the clear method ensures that the object can be reused without leftover data from previous operations, which is crucial for avoiding subtle bugs.

Proper copying of versions array in the copy constructor.

Ensuring that all relevant data members are correctly copied in the copy constructor is essential for the correct functioning of copied objects, especially when dealing with arrays or pointers.

CHANGELOG.md (1)

7-20: Updated changelog entries for version 19.0.1.

The changelog correctly summarizes the changes made in this version, including the introduction of new methods and the deprecation of unused ones. This is crucial for keeping the project's documentation up to date and providing clear communication to users and developers.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

ChristianTackeGSI · 2024-06-10T19:57:48Z

Can you extract the chore / deprecate part into its own PR? I like those and would like to get them finished and merged first.

ChristianTackeGSI · 2024-06-10T20:03:20Z

After having thought about this whole thing a bit (see
#1541 (comment)), I doubt that we actually should have yet another knob. (Yes, I proposed the knob thing, please excuse!) Not to mention, the "creation mode" as a bool sounds a bit strange. What is a truthy creation mode? What is a falsy creation mode?

And I really think by now, that the virtual init() is the customization point that users should use to handle all their cases about this part. We can try to make this even nicer / easier. All our cleanup PRs are going in that direction!

So alternative proposol (If you agrree): Repurpose this PR and drop the feat commit from it?

karabowi · 2024-06-11T09:58:53Z

@ChristianTackeGSI , I also do not like this knob name, I will work on this PR.

karabowi added 3 commits June 6, 2024 10:50

chore(parbase): Some cleanup of FairParSet

7f0aeb7

deprecate(parbase): Deprecate fill and store methodes in `FairPar…

51c73a4

…Set` These functions seem not be used in `FairRoot`. Deprecated them.

ChristianTackeGSI mentioned this pull request Jun 10, 2024

refactor(parbase): Improve logging in FairParSet #1551

Merged

1 task

karabowi added the in progress label Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `setCreationMode` to `FairParSet`#1552

Add `setCreationMode` to `FairParSet`#1552
karabowi wants to merge 3 commits intoFairRootGroup:devfrom
karabowi:add_creation_mode

karabowi commented Jun 6, 2024 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jun 6, 2024 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s) (Beta)

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

ChristianTackeGSI commented Jun 10, 2024

Uh oh!

ChristianTackeGSI commented Jun 10, 2024

Uh oh!

karabowi commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

karabowi commented Jun 6, 2024 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s) (Beta)

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

ChristianTackeGSI commented Jun 10, 2024

Uh oh!

ChristianTackeGSI commented Jun 10, 2024

Uh oh!

karabowi commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

karabowi commented Jun 6, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jun 6, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)