Add user data collections w/ basic types #213

gaede · 2021-09-01T09:31:16Z

BEGINRELEASENOTES

add possibility to store additional user data as collections of fundamental types in PODIO files
- uses std::vector<basic_type>
- stored in simple branch in root (and simple block in SIO)
- all fundamental types supported in PODIO (except bool) can be written
example code:

  auto& usrInts = store.create<podio::UserDataCollection<uint64_t> >("userInts");
  auto& usrDoubles = store.create<podio::UserDataCollection<double> >("userDoubles");
  // ...

  // add some unsigned ints
  usrInts.resize( i + 1 ) ;
  int myInt = 0 ;
  for( auto& iu : usrInts ){
    iu = myInt++  ;
  }
  // and some user double values
  unsigned nd = 100 ;
  usrDoubles.resize( nd ) ;
  for(unsigned id=0 ; id<nd ; ++id){
    usrDoubles[id] = 42. ;
  }

should replace [WIP] example code for a simple user defined float collection key4hep/EDM4hep#114 in a more efficient way

ENDRELEASENOTES

This should address most needs for storing user defined data in PODIO in an efficient way without any overhead. In ROOT every user data collection is stored directly in a branch of the given type and can be accessed directly.
No relations to or from the user defined collections are foreseen. Users need to keep track if indices, e.g. by running collections parallel to existing PODIO (EDM4hep) collections.

This PR should work now as an example for possible treatment of additional user data. Open questions:

should the set of basic_types be restricted ?
if so, to which types ?
some code cleanup/improvement needed (see suggestions below)
...

- works for int/float w/ ROOT

- can now use range based loops on collections

tmadlener · 2021-09-02T14:45:50Z

include/podio/UserDataCollection.h

+  typedef int    intData ;
+  typedef long   longData ;
+  typedef float  floatData ;
+  typedef double doubleData ;


Suggested change

typedef int intData ;

typedef long longData ;

typedef float floatData ;

typedef double doubleData ;

These seem to be unnecessary and cause the root problems in the CI (don't ask me why though).

Thanks, Thomas. This seems to do the trick (at least on my centos7 test system) However I do not understand why and more importantly how this can even work, as the branch for the user collection is defined (by the ROOTReader) as vector<doubleData> and doubleData is a non existing type w/o these typedefs !???

tmadlener · 2021-09-02T14:48:23Z

include/podio/UserDataCollection.h

+	&m_refCollections, // only need to store the ObjectIDs of the referenced objects
+	&m_vecmem_info } ;


Suggested change

&m_refCollections, // only need to store the ObjectIDs of the referenced objects

&m_vecmem_info } ;

nullptr,

nullptr } ;

Would allow it to not have to keep some unnecessary members for these parts in the class. This should be easily possible now, but I am not entirely sure whether everything that expects this already has the necessary nullptr check in place already.

This is what I had in initially - but it crashes in the SIOWriter as the SIOBlocks have:

//---- read ref collections ----- auto* refCols = collBuffers.references; for( auto* refC : *refCols ){ unsigned size{0}; device.data( size ) ; refC->resize(size) ; podio::handlePODDataSIO( device , &((*refC)[0]), size ) ; }

Should add a check for null pointer here ...

tmadlener · 2021-09-02T14:52:00Z

include/podio/UserDataCollection.h

+  class  UserDataTypes{
+  private:


Would it be possible to put all of this into a function instead of a singleton? In principle everything that is needed is a "map" of typeids to easier to read type names and that could also just be constructed statically (or potentially even static constexpr).

Probably. As this is still WIP I wanted to see how far we can take this approach and wanted to have a potentially extendible map (w/ adding a registerTypeName() method or similar...

tmadlener · 2021-09-02T14:54:41Z

include/podio/UserDataCollection.h

+    const std::map< const std::type_index, const std::string >_typeMap =
+    {
+      { std::type_index( typeid(int) ),   "podio::int"   },
+      { std::type_index( typeid(long) ),  "podio::long"  },
+      { std::type_index( typeid(float) ), "podio::float" },
+      { std::type_index( typeid(double) ),"podio::double"}
+    } ;


How restrictive do we want to be with this map? In principle I think it is a good idea to not be overly permissive, but I also think that a few more fundamental types might be useful (even if the listed ones probably cover > 90% of the use cases).

Open for discussion - this is just an initial set that probably would have to be extended by at least some short and unsigned, maybe fixed_size types. This depends if we can make the mechanism extendible, i.e. such that users can add additional types if needed or not. For SIO this should work easily - with ROOT I am not so sure due to the discionary, etc...

tmadlener · 2021-09-02T15:03:07Z

include/podio/UserDataCollection.h

+
+
+  /// Templated base class for storing std::vectors of basic types as user data in PODIO
+  template <typename BasicType>


This template should be constrained to only accept those types that we actually support via the mapping above. Currently it is possible to do something like

auto& coll = store.create<podio::UserDataCollection<unsigned>>("unsignedColl"); writer.registerForWrite("unsignedColl");

which compiles fine but crashes at some point at run-time. It should be possible to catch this at compile time already either via some static_assert or enable_if trickery.

Fully agreed, if we decide to only provide a fixed set of types - if not, then of course it cannot be done and we leave failure to runtime...

tmadlener · 2021-09-02T15:05:13Z

include/podio/UserDataSIOBlock.h

+
+  virtual void read(sio::read_device& device, sio::version_type /*version*/) override {
+    auto collBuffers = _col->getBuffers();
+    if (not _col->isSubsetCollection()) {


This check could be removed for the UserDataCollection, since it will always return true.

tmadlener · 2021-09-02T15:06:56Z

tests/read_test.h

+
+  auto& usrInts = store.get<podio::UserDataCollection<int> >("userInts");
+
+#if 0 //access via vector


Are these left over from testing, or are they actually required?

Sort of: testing - and as example how the underlying vector could be accessed, if needed. Can also remove eventually.
All this is still very much WIP ...

- now ROOT branches are of type vector<basic_type> and can be read w/ TBrowser correctly

tmadlener · 2021-09-06T12:04:32Z

src/ROOTWriter.cc

+      std::string valTypeName = coll->getValueTypeName() ;
+
+      auto collClassName = "vector<" + valTypeName + "Data>";
+
+      auto it = valTypeName.find("podio::User_") ;
+      if(it == 0 ){
+	valTypeName = valTypeName.substr( 12 , 1024 ) ; // rm 'podio_user_' prefix
+	 collClassName = "vector<" + valTypeName +">";
+      }


Should we consider adding a getDataTypeName function (or similar) to the podio::CollectionBase? It would make some of the string juggling easier to do for general use, and give us a customization point where these user classes could be handled more nicely. As far as I can tell the major use cases for getValueTypeName currently are:

Keys for the map inside the SIOBlockFactory

Creating the vector<XYZData> for the root writer and reader

So maybe it could even be a getDataVectorTypeName alongside the getValueTypeName (?)

vvolkl · 2021-09-07T11:18:40Z

Hi Frank,

I think the access to the underlying vector here is very nice! In my opinion the available data types are fine; they can always be expanded if necessary. Thomas' point for a compile time check is still a good one though.

- simplify rader's and writer's handling of type names

tmadlener

As already discussed during the meeting yesterday, I think this is a very nice addition and I like the implementation. I have left one comment that is not only about implementation details regarding the return value of getDataTypeName. I am not sure what others think there.

From a technical point of view I would then simply try to push as many failures to compile time rather than runtime, since we have a closed set of types that we want to support with this (the BUILTIN_TYPES sans std::string and the ALLOWED_FIXED_WIDTH_TYPES).

tmadlener · 2021-09-08T08:06:22Z

python/templates/Collection.h.jinja2

+  /// fully qualified type name of stored POD elements - with namespace
+  std::string getDataTypeName() const override { return std::string("{{ (class | string ).strip(':')+"Data" }}"); }


It looks like this will always be used like this

const auto bufferClassName = "std::vector<" + coll->getDataTypeName() + ">";

Should we simply make this return the full thing (and potentially give it a name that indicates that)? Or do we foresee any potential use cases where the Data name alone would be useful?

In a sense the fact that a vector<> is used here is an implementation detail of the ROOTReader/Writer and other future implementations (HDF5 ?) might make a different use of the underlying POD data type.
This is why I thought it would be more generic this way...

tmadlener

To implement a compile-time constraint on the usable types it would be possible to do something along the lines of the following: Define a std::tuple containing all supported types.

using SupportedUserDataTypes = std::tuple<
    int,
    float,
    long,
    double
    >;

Around that it is then rather straight forward to implement the necessary enable_if machinery to make compilation fail in case of unsupported types

namespace detail {
  /**
   * Helper function to check whether a type T is in a std::tuple<Ts...>
   */
  template<typename T, typename ...Ts>
  constexpr bool inTuple(std::tuple<Ts...>) {
    return (std::is_same_v<T, Ts> || ...);
  }

  /**
   * Compile time helper function to check whether the given type is in the list
   * of supported types
   */
  template<typename T>
  constexpr bool isSupported() {
    return inTuple<T>(SupportedUserDataTypes{});
  }
}

/**
 * Alias template to be used to enable template specializations only for the types listed in the
 * SupportedUserDataTypes list
 */
template<typename T>
using EnableIfSupportedUserType = std::enable_if_t<detail::isSupported<T>()>;

Usage:

template<typename T
         typename = EnableIfSupportedUserType<T>>
// class or function that should only be available if T is in SupportedUserDataTypes

tmadlener · 2021-09-08T12:04:07Z

include/podio/UserDataCollection.h

+  template <typename BasicType>
+  std::string userDataTypeName() { throw std::runtime_error( std::string(" unsupported type for UserDataCollection: ")
+							     + typeid(BasicType).name() ) ; }
+
+  template <>  std::string userDataTypeName<int>()   {return "int" ; }
+  template <>  std::string userDataTypeName<float>() {return "float" ; }
+  template <>  std::string userDataTypeName<long>()  {return "long" ; }
+  template <>  std::string userDataTypeName<double>(){return "double" ; }


These could be made into a

template<typename BasicType, typename = EnableIfSupportedUserType<BasicType>> constexpr const char* userDataTypeName(); template<> constexpr const char* userDataTypeName<int>() { return "int"; } // ... etc.

without any changes necessary to the rest of the code. To ease the adding of additional types one could introduce a small macro, but still with that we would have to spell out the list of supported times a second time (after the initial std::tuple).

#define ADD_USER_TYPE(type) template<> constexpr const char* userDataTypeName<type>( return #type; }

tmadlener

This looks good to me, I just have a few comments that are all more or less about the same thing: Trying to cut down the list of supported types to the minimum while keeping all the currently present functionality.

However, they can also be addressed later IMHO.

tmadlener · 2021-09-13T08:37:14Z

include/podio/UserDataCollection.h

+     */
+    using SupportedUserDataTypes = std::tuple<
+      int, long, float, double,
+      unsigned int, unsigned, unsigned long,


Suggested change

unsigned int, unsigned, unsigned long,

unsigned, unsigned long,

Since unsigned int == unsigned, I think it can be removed here (which would then also make it slightly more consistent with short, long, etc...). However, we are listing more duplicate types here with the fixed width types in any case. Maybe we should find the smallest list necessary and put some static_asserts in place here to make sure that the expectations are met.

tmadlener · 2021-09-13T08:40:22Z

include/podio/UserDataCollection.h

+  PODIO_ADD_USER_TYPE(int)
+  PODIO_ADD_USER_TYPE(long)
+  PODIO_ADD_USER_TYPE(float)
+  PODIO_ADD_USER_TYPE(double)
+  PODIO_ADD_USER_TYPE(unsigned)
+//  PODIO_ADD_USER_TYPE(unsigned int)
+  PODIO_ADD_USER_TYPE(unsigned long)
+  PODIO_ADD_USER_TYPE(char)
+  PODIO_ADD_USER_TYPE(short)
+  PODIO_ADD_USER_TYPE(long long)
+  PODIO_ADD_USER_TYPE(unsigned long long)
+//  PODIO_ADD_USER_TYPE(int16_t)
+//  PODIO_ADD_USER_TYPE(int32_t)
+//  PODIO_ADD_USER_TYPE(int64_t)
+  PODIO_ADD_USER_TYPE(uint16_t)
+//  PODIO_ADD_USER_TYPE(uint32_t)
+//  PODIO_ADD_USER_TYPE(uint64_t)


We could consider putting the fixed-width integers into the definitions here, because that would (almost) always give us a name without spaces (this seems to be a requirement for sio? at least there is the sio_name function that just replaces spaces with _).

The only exception is unsigned long long for which

static_assert(std::is_same_v<uint64_t, unsigned long long>);

fails, even though it seems to be 64 bits wide on most architectures. I am not entirely sure how to best handle that in case sio really does not handle spaces in its block names.

tmadlener · 2021-09-13T08:41:55Z

include/podio/UserDataCollection.h

+#include <vector>
+#include <typeindex>
+
+#define PODIO_ADD_USER_TYPE(type) template<> constexpr const   char* userDataTypeName<type>(){ return #type ; }


I think we should #undef this after we have used it to not give the impression that this is an extension point for users. (Even if a user tries to use it they will only run into a compilation error in any case)

tmadlener · 2021-09-13T08:51:43Z

src/SIOBlockUserData.cc

+  static SIOBlockUserData<int> _defaultintCollcetionSIOBlock ;
+  static SIOBlockUserData<long> _defaultlongCollcetionSIOBlock ;
+  static SIOBlockUserData<float> _defaultfloatCollcetionSIOBlock ;
+  static SIOBlockUserData<double> _defaultdoubleCollcetionSIOBlock ;
+  static SIOBlockUserData<unsigned> _defaultunsignedCollcetionSIOBlock ;
+  static SIOBlockUserData<unsigned int> _defaultunsignedintCollcetionSIOBlock ;
+  static SIOBlockUserData<unsigned long> _defaultunsignedlongCollcetionSIOBlock ;
+  static SIOBlockUserData<char> _defaultcharCollcetionSIOBlock ;
+  static SIOBlockUserData<short> _defaultshortCollcetionSIOBlock ;
+  static SIOBlockUserData<long long> _defaultlonglongCollcetionSIOBlock ;
+  static SIOBlockUserData<unsigned long long> _defaultunsignedlonglongCollcetionSIOBlock ;
+  static SIOBlockUserData<int16_t> _defaultint16_tCollcetionSIOBlock ;
+  static SIOBlockUserData<int32_t> _defaultint32_tCollcetionSIOBlock ;
+  static SIOBlockUserData<int64_t> _defaultint64_tCollcetionSIOBlock ;
+  static SIOBlockUserData<uint16_t> _defaultuint16_tCollcetionSIOBlock ;
+  static SIOBlockUserData<uint32_t> _defaultuint32_tCollcetionSIOBlock ;
+  static SIOBlockUserData<uint64_t> _defaultuint64_tCollcetionSIOBlock ;
+


Similar to the macro defining the userDataTypeName, I think we should be able to restrict ourselves to a smaller list (potentially focusing on the fixed width integer types). Since under the hood they will in any case call the the same userDataTypeName and register as the same type in the factory, e.g.

static_assert(std::is_same_v<podio::SIOBlockUserData<unsigned>, podio::SIOBlockUserData<uint32_t>>);

works fine. So even if users specify unsigned it should be enough if we have an uint32_t specialization.

- leave comments on potentiall eddressing this in more generic way in PODIO

gaede · 2021-09-14T15:30:16Z

Thanks @tmadlener - added your suggestions. As discussed privately, we might need a more generic treatment of ensuring consistency and size of fundamental types in PODIO eventually ...

hegner · 2021-09-21T08:16:11Z

include/podio/UserDataCollection.h

+    using SupportedUserDataTypes = std::tuple<
+      int, long, float, double,
+      unsigned int, unsigned, unsigned long,
+      char, short, long long,


can you quickly clean up the non-fixed width types?

I actually intentionally left them in, as this is what we currently support in PODIO. I think we should address the issue of 'set of supported types' in PODIO consistently in a dedicate PR (with sufficient documentation). What do you think?

I think for a new feature we should already start restricted. We can open up easily later in case. The other way round is more complicated.

Actually there is no restriction really due to the equivalence of the ususal named integer types and the fixed size ones. But I agree that this makes this feature much clearer and it already does what we probably want to do in all of PODIO ...

- this should be done for PODIO user data as well as discusse in EDM4hep meeting 21.09.21

gaede · 2021-09-21T14:52:12Z

Sure. Please merge after CI passed.

gaede added 6 commits August 30, 2021 17:15

first version of UserDataCollection

2a67501

- works for int/float w/ ROOT

some code cleanup

00a7c63

introduce UserDataTypes singleton

9654d74

add SIO I/O for UserDataCollections

448742d

add long/double types

c6bd9b0

add example w/ doubles and some vector wrappers

3e1da6c

- can now use range based loops on collections

tmadlener reviewed Sep 2, 2021

View reviewed changes

gaede added 3 commits September 3, 2021 13:56

rm typedefs for doubleData etc for testing

5f53315

fix names for vectors of user types for ROOT

b99056b

- now ROOT branches are of type vector<basic_type> and can be read w/ TBrowser correctly

rm checks for subsets in UserDataSIOBlock

a546f92

tmadlener reviewed Sep 6, 2021

View reviewed changes

vvolkl mentioned this pull request Sep 7, 2021

Propagate CollectionBuffer changes from https://github.com/AIDASoft/podio/pull/197 key4hep/k4FWCore#61

Merged

gaede added 2 commits September 7, 2021 18:02

add CollectionBasee::get[Data]TypeName() methods

fd26df9

- simplify rader's and writer's handling of type names

rm typedefs for UserDataCollection instantiations

b2fef33

tmadlener reviewed Sep 8, 2021

View reviewed changes

gaede added 4 commits September 10, 2021 14:58

compile time checks for user data types (T.Madlener)

3f34a83

add all fundamental types to UserDataCollection

0feb785

fix error: extra ';' [-Werror=pedantic]

63295e2

add missing <algorithm>

7ee9020

gaede changed the title ~~[WIP] Add user data collections w/ basic types~~ Add user data collections w/ basic types Sep 13, 2021

tmadlener reviewed Sep 13, 2021

View reviewed changes

add assertion on equivalence of basic types

2074487

- leave comments on potentiall eddressing this in more generic way in PODIO

gaede added 2 commits September 15, 2021 13:27

fix different size of long on MAC and Linux

734ec75

more fixes -sizeof and type equivalence MAC/Linux

d86e62f

hegner reviewed Sep 21, 2021

View reviewed changes

restrict to float, double and fixed size ints

b12f8f8

- this should be done for PODIO user data as well as discusse in EDM4hep meeting 21.09.21

hegner mentioned this pull request Sep 21, 2021

Document UserData interface #218

Closed

hegner merged commit f9e6c32 into AIDASoft:master Sep 21, 2021

tmadlener mentioned this pull request Sep 30, 2021

Add documentation for UserDataCollection #223

Merged

andresailer mentioned this pull request Oct 11, 2021

WIP: An example to show the User Defined data with int/float support key4hep/EDM4hep#117

Closed

1 task

tmadlener mentioned this pull request Oct 11, 2021

Add test to read a legacy (v00-13) root file #230

Closed

SanghyunKo mentioned this pull request Oct 28, 2021

Initialize 3D reconstruction feature HEP-FCC/dual-readout#21

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add user data collections w/ basic types #213

Add user data collections w/ basic types #213

gaede commented Sep 1, 2021 •

edited

Loading

tmadlener Sep 2, 2021

gaede Sep 3, 2021

tmadlener Sep 2, 2021

gaede Sep 3, 2021

tmadlener Sep 2, 2021

gaede Sep 3, 2021

tmadlener Sep 2, 2021

gaede Sep 3, 2021

tmadlener Sep 2, 2021

gaede Sep 3, 2021

tmadlener Sep 2, 2021

gaede Sep 3, 2021

tmadlener Sep 2, 2021

gaede Sep 3, 2021

tmadlener Sep 6, 2021

vvolkl commented Sep 7, 2021

tmadlener left a comment

tmadlener Sep 8, 2021

gaede Sep 8, 2021

tmadlener left a comment

tmadlener Sep 8, 2021 •

edited

Loading

tmadlener left a comment

tmadlener Sep 13, 2021

tmadlener Sep 13, 2021

tmadlener Sep 13, 2021

tmadlener Sep 13, 2021

gaede commented Sep 14, 2021

hegner Sep 21, 2021

gaede Sep 21, 2021

hegner Sep 21, 2021

gaede Sep 21, 2021

gaede commented Sep 21, 2021

		&m_refCollections, // only need to store the ObjectIDs of the referenced objects
		&m_vecmem_info } ;



		/// Templated base class for storing std::vectors of basic types as user data in PODIO
		template <typename BasicType>


		auto& usrInts = store.get<podio::UserDataCollection<int> >("userInts");

		#if 0 //access via vector

		/// fully qualified type name of stored POD elements - with namespace
		std::string getDataTypeName() const override { return std::string("{{ (class \| string ).strip(':')+"Data" }}"); }

	unsigned int, unsigned, unsigned long,
	unsigned, unsigned long,

Add user data collections w/ basic types #213

Add user data collections w/ basic types #213

Conversation

gaede commented Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvolkl commented Sep 7, 2021

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmadlener left a comment

Choose a reason for hiding this comment

tmadlener Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaede commented Sep 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaede commented Sep 21, 2021

gaede commented Sep 1, 2021 •

edited

Loading

tmadlener Sep 8, 2021 •

edited

Loading