-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address differences in ROOT5 and ROOT6 interfaces #47
Conversation
The wrapper function `TH1Helper::SetCanRebin(TH1 *h,int axis=0)` is introduced in c0bf719 See StRoot/StarRoot/TH1Helper.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FMS code in StQAMakerBase.cxx was provided to the QA group by people who are no longer in the collaboration. I don't expect this change to have any negative consequences for QA, so I'm ok with approving that one change, but I don't speak for the owners of the other codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great solution to what is likely to be a common problem in STAR codes.
The type of `TListIter::fCursor` and `TListIter::fCurCursor` (inherited by `StFileIter`) changes from `TObjLink*` to `std::shared_ptr<TObjLink>` in ROOT5 and ROOT6 respectively. In order to solve the problem with assignment operator we introduce a couple of overloads one of which is selected based on the actual type of the respective data members in `TListIter`.
`TH2* TH2::Rebin(...)` was introduced in ROOT6 overriding `TH1* TH1::Rebin(...)` It seems safe if we use covariant (`TH2*`) with the original return type (`TH1*`) for `TH2* StMultiH1F::Rebin(...)`
In ROOT6 the global pointer `TVirtualMC* gMC` was replaced by a macro `#define gMC (TVirtualMC::GetMC())` causing a compile time error when assigning to `gMC` I think is is safe to remove the assignment for the following reasons: - `TGeant3TGeo`, inheriting from `TVirtualMC`, is a singleton managing the global pointer `gMC` internally - `StarVMC/GeoTestMaker` is excluded from the build - `StarVMC/StarBASE` appears to be used only by an expert tool `StarVMC/StarBASE/macros/starbase.py`
Sorry for adding another commit to this thread but I think it belongs here. I believe TVirtualMC is your domain @klendathu2k and @perevbnlgov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be safe for our code.
It's good to see that ROOT and VMC are cleaning things up a bit. Getting rid of global pointers is a good thing. Singletons aren't much better... but GEANT3 wrappers are likely one of the few places where a singleton pattern makes any sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it should work. It's also in the cxx file, so not visible external to the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any mission critical codes which depend on this utility class. I note that TH1::Rebin is virtual in ROOT6, so does this solve a real problem? (I guess there is a compiler warning or other motivation here?) Regardless, don't have any issues with it.
Thanks very much for the feedback Jason ✌️
Yes, the introduction of the new
The old and new overriding chains are like this: |
I see....They added a Rebin() to TH2 in ROOT6 with a (slightly) different return type than the previously-inherited Rebin() from TH1. They didn't add one in TH3, so no impact on StMultiH2F. I don't think I (or anyone else) ever used the returned pointer, so I approve the change to StMultiH1F.h. |
Hopefully understood, but worth re-iterating: merging any changes like these that have the potential to affect FastOffline should wait until at least a couple weeks after the data-taking ends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked at the changes to StFileIter. The relevant upstream change is root-project/root@b5f8a69#diff-d78120b6a10b89cc75995c1c44f0a347a68f83b1c01cf5fbb3be8c543ec03b55. This PR changes the type and adds wrapper, and that seems to be sufficient.
Is there anything that would prevent us from creating a stable branch for fast offline and other online tools that would include only hand picked changes during the run? Sharing a common branch for bleeding edge changes is very common and in fact should be encouraged. On the other hand, preventing incremental development of new features during the run because the online chooses to use an unstable branch does not sound very productive. |
Hi, Dmitri
If the S&C management and others in the Collaboration (e.g. subsystem software coordinators) want DEV to be built from some other branch than "main" during Run operations, we can have a switch for AutoBuild.
…-Gene
|
Hi Gene: When you refer to the 'main' branch do you think of it as a stable or development branch? Picking the definition based on a specific time of year is at least confusing. |
Hi, Dmitri
If you're asking my perspective, I personally do not apply these terms to the code repository, but I do to the builds. DEV is a development build that we have leveraged for FastOffline during Run operations because we wanted the rapid code deployment scheme of DEV when necessary to support the Run operations, but that brings the requirement of being careful about what gets inserted by developers into the place from which DEV pulls its code. It is not a perfect scheme, and if you wish to change this established scheme, you need to have a clear proposal for how Run operations will be supported.
…-Gene
On Jul 6, 2021, at 11:30 AM, Dmitri Smirnov ***@***.******@***.***>> wrote:
Hi Gene: When you refer to the 'main' branch do you think of it as a stable or development branch? Picking the definition based on a specific time of year is at least confusing.
|
Gene, the data taking is over. Are you ok with merging these changes now? |
If there are no further comments I would like to merge this PR. We need to move on with ROOT6 |
I suggest getting SL21c out the door first.
-Gene
On Jul 13, 2021, at 10:52 AM, Dmitri Smirnov ***@***.******@***.***>> wrote:
If there are no further comments I would like to merge this PR. We need to move on with ROOT6
|
Good point. Agreed |
Gene, can you approve this PR (as you already indicated in the comments)? I think merging will happen automatically later. |
Aren't there already 2 approvals? I'd be interested to understand why additional approvals are needed if anyone knows why. -Gene |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to need my approval. I don't have any objections as long as these changes don't get merged into SL21c as we agreed, so here is that approval.
That implies that the approvals from Jason and Dmitry K. were meaningless. Correct? |
Can't speak for others but for me it is important to see the reaction from other contributors especially when I know that they spent time looking over the changes. So, definitely not meaningless. |
OK, reasonable point. I agree that I would've withheld my approval if there was lesser support from such others. |
Perhaps, I should have mentioned in the closing comment that tagging or branching can be done from any past commit in the existing history. So, any changes on the main branch after the tag or branching point can be ignored |
Perhaps you could’ve waited just a few more hours (was it that urgent?) as we had agreed. Please let me know how to get the version hash from before today’s merges. Thanks,
…-Gene
On Jul 19, 2021, at 11:54 AM, Dmitri Smirnov ***@***.***> wrote:
Perhaps, I should have mentioned in the closing comment that tagging or branching can be done from any past commit in the existing history. So, any changes on the main branch after the tag or branching point can be ignored
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#47 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AUK2OBPLDNPVVMNC6CFCYRDTYRDCVANCNFSM47TC5DPQ>.
|
There is no point in waiting if all we want to include in the release is already in the repo |
We could make what @plexoos suggests to be more formal by designating a stabilization branch that could receive backports for only the release-critical fixes |
The main idea behind the proposed changes is to have the code that can seamlessly work with ROOT5 and ROOT6