Update Components/ port drawing

[mperrinel]

The Components should draw themselves as boxes. The ports should draw themselves as lines between components. The Components will just stay the same color the whole time for now.

[jjwilke]

We want to configure the geometry via parameters in the Python file:

portStat = comp.addStatistic("vtk", ....)
portStat.addParams({
  "shape" : "box",
  "size" : [1,1,1],
  "origin" : [0,0,0],

OR

portStat = comp.addStatistic("vtk", ...)
portStat.addParams({
  "shape" : "line",
  "begin" : [0,0,0],
 "end" : [1,1,1],
})

[jjwilke]

If we wanted to draw a component (even if it's not changing color or doing anything):

emptyBox = comp.addStatistic("vtk", ...)
emptyBox.addParams({
  "shape" : "box",
  "size" : [...],
  "origin" : [...],
})

[jjwilke]

Use this code example as a guide:

// this struct should not depend on VTK or any specific library
// use common base class for the topology/geometry
struct Shape3D {
  enum Shape {
    Box,
    Line,
    ...
  }
  Shape shape;
  Shape3D(Shape s) :
    shape(s)
  { 
  }
};

struct Box : Shape3D {
  Box3D(double x_origin, double y_origin, double z_origin,
         double x_extent, double y_extent, double z_extent) :
      Shape3D(Box)
      ...
 {
 }
};

 void drawExodus(ExodusOutput* out) override {
  vtkSmarkPointer<....> points;
  std::set<VTKShape*> shapes;
  for (Shape3D* shape : shapes){
    switch(shape->shape){
      case Shape3D::Box:
        //draw a box
      case Shape3D::Line:
        //draw a line
    }
  }
 }
}

[jjwilke]

MultiStatistic<...>* stat = registerStatistic(...) //I do this
VTKStat* stat = registerStatistic(...) //I DO NOT do this

// this struct should not depend on VTK or any specific library
struct Stat3DViz { //we want common functionality in base classes like this
  //possibly we don't use VTK
  Stat3DViz(SST::params& params){
    my_shape_ = get_shape_from_params(params);
  }

 private:
  Shape3D* my_shape_;
};

//don't worry about this for now - I can make this change later
struct StatVTK : public Stat3DViz {
};

[mperrinel]

@jjwilke

I finished a first version of the Shape construction for the StatVTK:

The new source file stat3Dviz.h has been added and contains the corresponding Stat3DViz class which holds a name and a pointer on a Shape3D class. A Stat3DViz constructor takes a Params as parameter which is parsed during the construction to detect which sub class (Line3D or Box3D) of Shape3D have to be used for its Shape3D member variable.

The Shape3D class defines a Shape enum which has two possible values (Box and Line) and have a member variable of the enum type. This class defines two pure virtual methods (isBox() and isLine()) which permit to check the nature of the shape member variable.

A new member variable of type Stat3DViz have been added the StatVTK class. The Stat3DViz is constructed by the StatVTK constructor. Its name member variable is defined by the getCompName() method of the Statistic.

The StatVTK::outputExodus has been updated to take the traffic intensity map and the new Stat3DViz set as a parameters. A loop is done on this set to fill the vtkPoints :

 for (const auto& vtkStat3dViz : vtkStat3dVizSet) {
      Shape3D *shape = vtkStat3dViz.my_shape_;
      if (vtkStat3dViz.my_shape_->isBox()) {
          if (Box3D * box = dynamic_cast<Box3D *> (shape) ) {
              // Fill the vtkPoints
              points->SetPoint(0 + i, box->x_origin_, box->y_origin_, box->z_origin_);
              points->SetPoint(1 + i, box->x_origin_ + box->x_extent_, box->y_origin_, box->z_origin_);
              ...

I'm not really happy with the Shape3D architecture because the shape enum is redundant with the real type of the child class. In the code example :

  if (vtkStat3dViz.my_shape_->isBox()) {
          if (Box3D * box = dynamic_cast<Box3D *> (shape) ) {

These two conditions test exactly the same Idea and should be simplify to one condition. Since we need to have a Box3D pointer to retrieve the coordinate, the dynamic_cast condition should be enough. In addition, I'm not really comfortable with the idea that a parent class (Shape3D) should holds an enum that lists all the possibilities of its child classes (Line3D and Box3D). That look like an ambiguity between the composition and the inheritance concepts.

I need to log some errors in case of the shape type isn't recognised. What is the best way to do that ?

Thanks,

[jjwilke]

Exits are done with getSimulation()->getSimulationOutput().fatal(...). You can see some examples in the sst-core code if you look for out.fatal(....).

I was imagining the Shape3D would just be a POD type. It's not really a C++ pattern (more common in C). You would static_cast based on the enum. I don't see a good way to have a generic shape library that uses inheritance in the "correct" way. VTK will have its own allowed shapes and how to draw them. Another library will have different allowed shapes and how to draw them...

switch(my_shape_->kind)
{
  case Shape3D::Line:
     drawVtkLine(static_cast<Line3D*>(my_shape_));
     break;
}

[mperrinel]

@jjwilke I analyzed the StatVTK class which is on the vtk_stat.h,.cc files and how we can make a correct isolation of the VTK code. The current state shows that :

1. There is absolutely no VTK code in the header file vtk_stat.h, which includes the traffic_event structure used for the traffic intensity collection and the StatVTK itself.
2. On the vtk_stat.cc :
   1. The collection itself doesn't need VTK
   2. The VTK Code is only used to create a VTK pipeline, used by the vtkTrafficSource that handles the temporal data and the vtkExodusWriter which creates the final exodus file. This code is completely isolated into the outputExodus Statistic method of the vtk_stat source files.

To improve the isolation of the VTK part, I suggest to :

1. Keep the no VTK code on the vtk_stat.h file, and to extend it to the vtk_stat.cc file. For that, we have to move the static outputExodus method from vtk_stat to vtkTrafficSource
2. Rename the vtk_stat filename to stattraffic. This class will be available all the time and doesn't need the VTK dependency anymore.
3. Rename the StatVTK class to TrafficStatistic :

from

class StatVTK : public MultiStatistic<uint64_t, int, double>

to

class TrafficStatistic : public MultiStatistic<uint64_t, int, double>

That way, the new TrafficStatistic (the old StatVTK itself) becomes a new statistic for SST without necessarily needing to enable VTK.

Currently, the only way to output it is to use the StatisticOutputExodus which call the static outputExodus method. With the new API, this method will be defined into the vtkTrafficSource class with all the complete VTK code.
As a result, the StatisticOutputExodus still call the static method, but from the vtkTrafficSource instead of the StatVTK.: from

void StatisticOutputEXODUS::endOfSimulation()
{
    StatVTK::outputExodus(m_FilePath, std::move(m_traffic_progress_map),
                          std::move(m_stat_3d_viz_list_)
                          );

    // Close the file
    closeFile();
}

to

void StatisticOutputEXODUS::endOfSimulation()
{
    vtkTrafficSource::outputExodus(m_FilePath, std::move(m_traffic_progress_map),
                          std::move(m_stat_3d_viz_list_)
                          );

    // Close the file
    closeFile();
}

The StatisticOutputEXODUS, still need to be included into the VTK extension of the simulator. But we can imagine, as a second step, a way to call the static outputExodus method using a factory and a key word coming from the python file :

void StatisticOutputEXODUS::endOfSimulation()
{
    ExodusFactory::output(m_FilePath, std::move(m_traffic_progress_map),
                          std::move(m_stat_3d_viz_list),
                          outputType // outputType is the key string or enum that permits to retrieve the vtkTrafficSource::outputExodus or another one outputExodus method
                          );

    // Close the file
    closeFile();
}

Does that make sense for you ?

[jjwilke]

struct SortedIntensityEvent {
  int cellId;
  uint64_t time;
  double intensity;
}

Inside StatisticOutputEXODUS::output:

      StatVTK* vtkStat = dynamic_cast<StatVTK *>(statistic);
      int cellId = m_stat_3d_viz_vector.size();
      m_stat_3d_viz_vector.push_back(vtkStat->getStat3DViz());
      for (const auto & eventI : vtkStat->getEvents()) {
        m_traffic_progress_map.emplace(event.time, event.intensity, cellId);
      }

We should ideally be able to make cellId here match the cell ID in vtkTrafficSource. Otherwise we can maintain a mapping. Instead of looking up each Stat3DViz by string name, we should look them up by a unique cell ID.

[mperrinel]

@jjwilke Thanks, I'm working on it. I have one question, on the method

void IntensityStatistic::addData_impl(uint64_t time, int port, double intensity) 

If we change the intensity_eventmap from a map to a vector, does that mean that we can assume that we will never have two intensities for the same time. I think that can be the case for some simulation :

SnapprOutPort::logQueueDepth()
...      traffic_intensity->addData(last_queue_depth_collection.time.ticks(), number_, queueLength());

If that is the case, I think to keep an order on the time event is necessary to find the intensity_event and to be able to update its intensity value.

 auto it = intensity_event_map_.find(time);
    if (it == intensity_event_map_.cend()) {
        intensity_event event(time, port, intensity, this->getCompName());
        intensity_event_map_.emplace(event.time_, event);
    }
    else {
        std::cout << "IntensityStatistic::addData_impl time founded " << time << " "<< port << " "<< intensity<< std::endl;
        it->second.color_ = intensity;
    }

It is also possible to keep two different events with the same time and to filter the latest during the aggregation, but with this solution, we also must check from witch component it comes from because we can have two different events with the same time if they are attached to different component/Statistic object.

What do you think about that ?

[jjwilke]

If we have two events with the same time, then there's no order we can assign to them. It doesn't matter then what order they get pushed back in the vector. They should just both be added. This will be a very uncommon case so there's no reason to design around it.

When we sort the events at the end of the simulation to make the Exodus file then, yes, we would need to store the event with a stat ID to make sure events at the same time on different stat objects are both included.

The component ID isn't enough to guarantee a unique stat. We need each stat object to be have its own unique ID since a component or subcomponent could have multiple different stat objects.