[WIP] : preliminary datamodels for SDD bank #103

goliviero · 2018-05-22T16:13:12Z

Changes proprosed in this pull request:

Change SDD datamodels 'architecture'
Remove Sim Digi event data and readout data.
Architecture is : 1 SDD is composed by 3 collection :
collection of calo digitized hits
collection of tracker digitized hits
collection of trigger data

Does this pull request fix any reported issues?

Related to issue Design and Implement Data Models For Raw/Calibrated Data #74

Additional comments or information

Work is still in progress and can change and evolve later.
PR is open in order to track commits and evolution.
Feedback is required and code can be update.

- Change SDD datamodels 'architecture' - Remove Sim Digi event data and readout data. - Architecture is : 1 SDD is composed by 3 collection : - collection of calo digitized hits - collection of tracker digitized hits - collection of trigger data

drbenmorgan · 2018-05-23T12:27:50Z

Hi @goliviero, many thanks for starting this. I'll have some further comments once I've had a chance to look through things in more detail. In the meantime, could you review the formatting of the code, e.g.

https://github.com/goliviero/Falaise/blob/35ee43460886e2dec3747c3c0ebf10a5410f1764/source/falaise/snemo/datamodels/sim_calo_digi_hit.h#L18-L47

The use of tabs/8 spaces for indent is slightly too big (see https://github.com/SuperNEMO-DBD/Falaise/blob/develop/source/falaise/version.h.in and https://github.com/SuperNEMO-DBD/Falaise/blob/develop/source/falaise/version.c for the recommended layout), but more importantl is to keep things lined up!

- Also fix indentation

goliviero · 2018-05-23T13:27:14Z

source/falaise/snemo/testing/test_snemo_datamodel_sim_digi_data.cxx

+    {
+      itcalo->get().tree_dump();
+    }
+
  datatools::data_writer writer("test_snemo_datamodel_sim_digi_data.xml",
                                datatools::using_multi_archives);
  writer.store(sddWrite);


This test is broken during the READ (not the store). The code compiles but, here is the error during execution :
[fatal:int main(int, char**):45] [void datatools::data_reader::basic_load(Data&) [with Data = snemo::datamodel::sim_digi_data]:684: input stream error]
So I add a new handle to the calorimeter collection and then take the reference to the calo hit in order to update it. I do it 3 times for 3 calo. Then I push back them in the calorimeter collection. The store method of the writer works because it produce the file.
The execution error happens during the reader.read(sddReadback). I don't understand why the reader can not read and restore the snemo::datamodel::sim_digi_data object.
Fun fact : the test works at execution if I push back only 1 handle (so with 1 calo_digi hit).
So help is needed here !

Error found : it was because of boost::serialization on uninitialized boolean. It can store boolean even if the value is not 0 or 1 but it can not load / restore the value and it threw an exception.
In the source code of boost serialization it says :

// trap usage of invalid uninitialized boolean which would // otherwise crash on load.

But the error is not manage here during the store, only in the load. Maybe an 'error' is made in boost because it should manages this error during the storage.

drbenmorgan · 2018-05-23T13:02:58Z

source/falaise/snemo/datamodels/sim_calo_digi_hit.h

-  /// Constructor
-  sim_calo_digi_hit();
+		/// \brief Simulated calo digitized hit.
+		class sim_calo_digi_hit : public mctools::digitization::sampled_signal


Given the simplicity of this data (I'm going from RDD/SDD in DocDB-4612), could the inheritance be replaced with composition here, e.g.

class sim_calo_digi_hit { ... private: geomtools::geom_id geomID_; geomtools::geom_id elecID_; ... std::vector<int16_t> waveform; // maybe even std::array if size known ... sample frequency etc? };

Or, if attributes go along with the waveform, then define an additional struct to organise this? The mctools object brings a lot of complexity the requirement for which is not obvious yet. Composition can also make persistency easier, and schema evolution more robust.

drbenmorgan · 2018-05-23T13:11:56Z

source/falaise/snemo/datamodels/sim_digi_data.cc

+    sim_digi_data::~sim_digi_data()
+    {
+      return;
+    }


No return needed.

drbenmorgan · 2018-05-23T13:12:01Z

source/falaise/snemo/datamodels/sim_digi_data.cc

+      _tracker_digi_hits_.clear();
+      _trigger_data_.clear();
+      _auxiliaries_.clear();
+      return;


The clear() calls are superfluous here I think, as the implicit default construction should create the necessary initialization.

The return is also superfluous.

drbenmorgan · 2018-05-23T13:12:12Z

source/falaise/snemo/datamodels/sim_digi_data.cc

+      _trigger_data_.clear();
+      _auxiliaries_.clear();
+      return;
+    }


What's the rationale (use cases) for resetting the data? Should data, once created, be read-only?

drbenmorgan · 2018-05-23T13:13:42Z

source/falaise/snemo/datamodels/sim_digi_data.h

+
+		private:
+
+			datatools::properties _auxiliaries_;  //!< Auxiliary properties


Please explain the purpose and use cases of this data member. What data does it represent?

drbenmorgan · 2018-05-23T13:16:28Z

source/falaise/snemo/datamodels/sim_tracker_digi_hit.cc

+    {
+      return;
+    }
+


As in other places, default construction (even the C++11 = default mechanism) should be sufficient here, and returns are pointless code.

drbenmorgan · 2018-05-23T13:17:33Z

source/falaise/snemo/datamodels/sim_tracker_digi_hit.cc

+      reset_cathode_R5();
+      reset_cathode_R6();
+      return;
+    }


As with the calo data, what is the rationale and use cases for reseting the data here? Why would it change later?

drbenmorgan · 2018-05-23T13:20:08Z

source/falaise/snemo/datamodels/sim_tracker_digi_hit.cc

+      _elec_id_ = a_gid;
+      return;
+    }
+


Again, surely an electronic id is really an invariant of the instance once created? Why would it change after the data is created?

I think I want to keep this interface for EID because a sim_tracker_digi_hit can be created with a given GID and then the EID will be set once the GID has been converted into an EID by the electronic mapping.
A sim_tracker_digi_hit and a sim_calo_digi_hit have 2 geomtools::geom_id :

A Geometric ID (cell ID with Module/Row/Layer etc... for example)

And an Electronic ID (channel ID with : Crate:Board:Feast:Channel)
It is useful to have these 2 IDs during the digi process and maybe it will be useful later so we store them both

drbenmorgan · 2018-05-23T13:28:09Z

source/falaise/snemo/datamodels/sim_tracker_digi_hit.cc

+      DT_THROW_IF(!has_anode_R0(), std::logic_error, "Missing anode t0");
+      return _anode_R0_;
+    }
+


I'm confused by this "store mask" pattern for each timestamp. The class provides an INVALID_TIME constant used as a marker of an unmeasured/bad value, why not just use this and let the client of the data decide what to do? The has_XXX interfaces can then go (same for the reset) and, the get_/set_ methods are simplified.

I don't think the get_... should they and throw exceptions. The presence of invalid data isn't an "execeptional" case here.

You're right, the value should be stored even if it's an invalid time. The invalid time is an information.
And it will garanty that there are 7 times recorded even if there is no data.

I'll remove the DT throw too because we want to access to timestamps even if they are invalid

drbenmorgan · 2018-05-23T13:32:15Z

source/falaise/snemo/datamodels/sim_tracker_digi_hit.h

-  static const int16_t OVERFLOW_TIME = std::numeric_limits<int16_t>::min() + 1;
-  static const int16_t UNDERFLOW_TIME = std::numeric_limits<int16_t>::max();
+		/// \brief Simulated tracker digitized hit.
+		class sim_tracker_digi_hit : public geomtools::base_hit


As with the calo, I think simple composition rather than inheritance should be explored here. Can inheritance from base_hit be replaced by a "geom_id" data member?

- Fix failing test. Have to initialized boolean for boost serialization - Sim calo and tracker hits should inherit from a geomtools base hit conceptually - Sim calo digi hit embeded a vector representing the waveform. The size is not static so it is a vector and not an array

- Also fill the serialization part

drbenmorgan · 2018-06-27T15:07:59Z

@goliviero, just to note and cross-reference #110 here. We are seeing some issues with Bayeux/Boost serialization with clang. Please ensure that comprehensive serialize/deserialize tests are provided to see if these objects are affected.

drbenmorgan · 2019-03-18T10:35:53Z

@fmauger, @lemiere, please check on this PR and determine if it is still required on not now that the datamodels and ASB/Digitization modules no longer have a maintainer.

drbenmorgan · 2019-05-07T09:47:34Z

Bump: @fmauger, @lemiere, please check on this PR and determine if it is still required on not now that the datamodels and ASB/Digitization modules no longer have a maintainer.

All I need is a yes/no answer.

drbenmorgan · 2019-05-21T10:10:53Z

Closing due to lack of response

goliviero added the enhancement label May 22, 2018

drbenmorgan requested review from drbenmorgan, yramachers, macolino and lemiere May 23, 2018 12:27

Add handles to SDD for calo / tracker and trigger collections

cba49cd

- Also fix indentation

goliviero commented May 23, 2018

View reviewed changes

drbenmorgan reviewed May 23, 2018

View reviewed changes

goliviero added 6 commits May 25, 2018 18:06

Add interface to simulated calo digi hit

871285a

Add standard interface to SDD to add calo / tracker / trigger hits.

3024b94

Minor modification on data types and attributes

5186cc9

Add getter and setter to sim trigger digi data

4c309ca

- Also fill the serialization part

Fill trigger digi data class

883a45e

add interface

b51cb7d

drbenmorgan assigned fmauger and lemiere Mar 18, 2019

drbenmorgan closed this May 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] : preliminary datamodels for SDD bank #103

[WIP] : preliminary datamodels for SDD bank #103

goliviero commented May 22, 2018

drbenmorgan commented May 23, 2018

goliviero May 23, 2018 •

edited

Loading

goliviero May 24, 2018

drbenmorgan May 23, 2018

drbenmorgan May 23, 2018

drbenmorgan May 23, 2018

drbenmorgan May 23, 2018

drbenmorgan May 23, 2018

drbenmorgan May 23, 2018

drbenmorgan May 23, 2018

drbenmorgan May 23, 2018

goliviero May 23, 2018

drbenmorgan May 23, 2018

goliviero May 23, 2018

drbenmorgan May 23, 2018

drbenmorgan commented Jun 27, 2018

drbenmorgan commented Mar 18, 2019

drbenmorgan commented May 7, 2019

drbenmorgan commented May 21, 2019


		private:

		datatools::properties _auxiliaries_; //!< Auxiliary properties

[WIP] : preliminary datamodels for SDD bank #103

[WIP] : preliminary datamodels for SDD bank #103

Conversation

goliviero commented May 22, 2018

Additional comments or information

drbenmorgan commented May 23, 2018

goliviero May 23, 2018 • 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

drbenmorgan commented Jun 27, 2018

drbenmorgan commented Mar 18, 2019

drbenmorgan commented May 7, 2019

drbenmorgan commented May 21, 2019

goliviero May 23, 2018 •

edited

Loading