Skip to content
Snippets Groups Projects

Revert some interface changes of THistSvc

Merged Revert some interface changes of THistSvc
merrenst/Gaudi:THistSvc-180131 into master
Merged Martin Errenst requested to merge merrenst/Gaudi:THistSvc-180131 into master

The recent interface changes in THistSvc would require too much manual intervention for quick adaption in Athena. For this reason I reverted some of the changes and added (deprecated) methods to preserve most of the old interface. This also means, that there are more StatusCodes again, opposed to returning nullable types to communicate failures.

As a rule, the THistSvc is expected to take ownership of the managed histograms, but a very common use case is to register a histogram pointer and continue to work on it afterwards. The interface now offers methods to do exactly that in one call, while communicating ownership transfer to the service to some extend.

This affects only histogram methods. regTree, getTree, regGraph and getGraph aren't used that often in Athena (< 100 times), so that I should be able to migrate those to unique_ptrs within a couple of days on my own.

The deprecation attribute is introducing one additional compiler warning in Gaudi (ITHistSvc.h -> THistSvc.h), so how should I address that in the future? I could remove the attribute from the interface and only leave a comment there. This way, only clients of the THistSvc implementation would be marked during compile time. (None in Gaudi; O(1000) times in Athena without any scripted migration to a non-deprecated method)

To prepare the THistSvc for future rewriting efforts (with ROOT 7 histograms in mind), I have changed the order of functions and added many comments. Hopefully this makes the MR/commits not too bloated for everyones taste.

Edited by Marco Clemencic

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Martin Errenst added 3 commits

    added 3 commits

    • c000ca11 - Initialize pointer in THistSvc tests
    • 13008b44 - Add GAUDI_API back to ITHistSvc
    • 59586ba1 - Initialize all pointers in THistSvc

    Compare with previous version

  • Martin Errenst added 1 commit

    added 1 commit

    • 16b9cd7d - Apply clang formatting patch from CI

    Compare with previous version

  • Martin Errenst resolved all discussions

    resolved all discussions

  • Martin Errenst added 1 commit

    added 1 commit

    • bf3534a6 - Hide deprecation warning behind ATLAS includeguard

    Compare with previous version

  • Martin Errenst marked as a Work In Progress

    marked as a Work In Progress

  • Martin Errenst added 1 commit

    added 1 commit

    • 368f0434 - Let the thistwrite ignore specific file sizes

    Compare with previous version

  • Martin Errenst
    Martin Errenst @merrenst ·
    Author Contributor

    @clemenci I have checked the debug reference file again and my local debug build is not picking up the .ref.dbg, but .ref reference. I do not really understand the mechanism how .dbg should be picked up automatically in a debug build. Do you have some recommendations for that?

  • Martin Errenst added 1 commit

    added 1 commit

    Compare with previous version

  • Martin Errenst added 1 commit

    added 1 commit

    Compare with previous version

  • Martin Errenst unmarked as a Work In Progress

    unmarked as a Work In Progress

  • C
    Marco Clemencic @clemenci ·
    Owner

    Sorry for the late answer.

    The right ref file is picked up using $CMTCONFIG (or $BINARY_TAG), so if you build using BINARY_TAG=*-opt and CMAKE_BUILD_TYPE=Debug, it will still use the "opt" version of the reference file.

  • Martin Errenst added 1 commit

    added 1 commit

    • a977cc34 - Change test regex also for reading

    Compare with previous version

  • Martin Errenst
    Martin Errenst @merrenst ·
    Author Contributor

    Thanks Marco! The BINARY_TAG was the missing piece for me! I've fixed the read and write test locally for me and from my side everything is ready to be merged!

  • Charles Leggett
    Charles Leggett @leggett ·
    Owner

    @merrenst : could you please squash these into 1 commit?

  • Martin Errenst marked as a Work In Progress

    marked as a Work In Progress

  • Martin Errenst added 1 commit

    added 1 commit

    • 83c5e66b - Update ITHistSvc for better Athena migration

    Compare with previous version

  • Martin Errenst
    Martin Errenst @merrenst ·
    Author Contributor

    @leggett done!

  • Martin Errenst added 1 commit

    added 1 commit

    • 5afd894c - Update ITHistSvc for better Athena migration

    Compare with previous version

  • Martin Errenst added 2 commits

    added 2 commits

    • 83c5e66b - Update ITHistSvc for better Athena migration
    • 21264e1f - Add documentation README to THistSvc folder

    Compare with previous version

  • Martin Errenst added 1 commit

    added 1 commit

    • 98965b7c - Fix bug in deReg and add test

    Compare with previous version

  • Martin Errenst added 1 commit

    added 1 commit

    • d9a07c28 - Fix bug in deReg and add test

    Compare with previous version

  • Marco Clemencic unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Marco Clemencic mentioned in commit clemenci/Gaudi@90280e81

    mentioned in commit clemenci/Gaudi@90280e81

  • Marco Clemencic mentioned in commit clemenci/Gaudi@8b0ac931

    mentioned in commit clemenci/Gaudi@8b0ac931

  • Martin Errenst added 1 commit

    added 1 commit

    • fa80d699 - Remove delete in deReg (giving up ownership)

    Compare with previous version

  • Marco Clemencic approved this merge request

    approved this merge request

  • Marco Clemencic merged

    merged

  • Marco Clemencic mentioned in commit f6504322

    mentioned in commit f6504322

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
    • Please register or sign in to reply