Skip to content
Snippets Groups Projects

Draft: Modernisation of FileMetaData

This MR modernises implementation of some FileMetaData-related classes, mostly by replacing function overloading with templates (making use of concepts).

It will facilitate adding more payload to FileMetaData (e.g. from EventStreamInfo).

/cc @gemmeren

Merge request reports

Members who can merge are allowed to add commits.

Checking pipeline status.

Approval is optional
Merge blocked: 2 checks failed
Merge request must not be draft.
Unresolved discussions must be resolved.

Merge details

  • The source branch is 1606 commits behind the target branch.
  • 1 commit and 1 merge commit will be added to main (squashes 2 commits).
  • Source branch will be deleted.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
149 case MetaDataType::simFlavour:
150 return "simFlavour";
151 case MetaDataType::isDataOverlay:
152 return "isDataOverlay";
153 case MetaDataType::mcCampaign:
154 return "mcCampaign";
155 case MetaDataType::generatorsInfo:
156 return "generatorsInfo";
157 case MetaDataType::dataYear:
158 return "dataYear";
159 case MetaDataType::END:
160 return "END";
161 default:
162 return "Unknown";
163 }
164 }
  • Comment on lines +127 to +164

    I think I understand the idea here, but I still don't like putting all this code into the header. :thinking:

    Could this not be a private / hidden function in the .cxx file? Then you could still make it into a constexpr.

  • Unless I'm missing something, constexpr implies that the function is inline, so must be defined in every translation unit where it's used. In practice, I got a bunch of warnings from the clients, like this:

    warning: inline function 'static constexpr std::string xAOD::FileMetaData_v1::metaDataTypeToString(MetaDataType)' used but never defined

    when I simply moved declaration to .cxx file.

    Do you have any suggestion regarding this?

  • :thinking: If you need this function in the template functions, then yeah, the implementation needs to be in the header as well.

    I was just hoping that we may not need this function in the header. But with the current code proposal, we probably do...

  • Please register or sign in to reply
  • 155 case MetaDataType::generatorsInfo:
    156 return "generatorsInfo";
    157 case MetaDataType::dataYear:
    158 return "dataYear";
    159 case MetaDataType::END:
    160 return "END";
    161 default:
    162 return "Unknown";
    163 }
    164 }
    165
    166 template <typename T, typename V>
    167 requires(std::is_same_v<T, xAOD::FileMetaData_v1::MetaDataType> ||
    168 std::convertible_to<T, std::string_view>) &&
    169 FileMetaDataCompatible<V> bool
    170 value(T type, V& val) const {
    • I'm not the biggest fan on first look. :frowning: The finite variable types was not a shortcoming of the design. It was/is a feature. Since we can't be super flexible with the auxiliary variables that we use. (We need dictionaries for all persistent types after all.)

      Wouldn't it be easier to just add a couple more types to the list supported by xAOD::FileMetaData and be done with it? :thinking:

    • Thank you very much for the review! I appreciate the comment about the reasoning for the original design.

      Since I was experimenting with adding more payload to FileMetaData, I found that duplicating code with more almost identical functions and macros feels unnecessary, and may be simplified with templates/concepts. But no strong opinion given your comment about the original design.

      What do you mean by

      Wouldn't it be easier to just add a couple more types (...)

      ? IIUC, in current design one needs to declare for each new type the relevant accessor macro, update FileMetaData_v1::compareWith(), and implement value/setValue, correct?

    • Yes, I think right now a fair amount of new code would need to be added. But we could also think of a design in which the public interface of the class is only dealing with a fixed set of types, but behind the scenes we use a bunch of templating to implement the necessary functions.

      Just spit-balling here, but I could even imagine grouping all of the type-specific functions into a class like xAOD::FileMetaDataValueAccess<T>, and then defining the interface class like:

      class FileMetaData_v1 : public SG::AuxElement,
                              public FileMetaDataValueAccess<int>,
                              public FileMetaDataValueAccess<float>,
                              ...

      Since in such a setup we would only want to use that helper class with a finite number of template arguments, we could hide its implementation into the Root/ directory.

      It's really just that I don't want our EDM to be "super templated". :thinking: That goes a bit against having an EDM in the first place. Since at one point we might as well use SG::AuxElement as is, since that also allows us to access any type with any name in a generic way. :thinking:

    • Thanks for the suggestion, I may try that.

      I see your point about templates in EDM. I just thought that concepts are a nice way to improve readability here, and by putting constraints on the template parameters we do not allow to access any type in a generic way.

    • @akraszna could you please have a look if we can proceed with this MR or !73111 ?

    • Please register or sign in to reply
  • :white_check_mark: CI Result SUCCESS (hash dbe3bcdf)

    Athena AthSimulation AthGeneration AnalysisBase AthAnalysis
    externals :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    cmake :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    make :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:
    tests :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark: :white_check_mark:

    Full details available on this CI monitor view. Check the JIRA CI status board for known problems
    :white_check_mark: Athena: number of compilation errors 0, warnings 0
    :white_check_mark: AthSimulation: number of compilation errors 0, warnings 0
    :white_check_mark: AthGeneration: number of compilation errors 0, warnings 0
    :white_check_mark: AnalysisBase: number of compilation errors 0, warnings 0
    :white_check_mark: AthAnalysis: number of compilation errors 0, warnings 0
    :pencil: For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 12142] (remote access info)

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