VectorComposite::getStructure() sorted string

I've thought quite a bit about this.

Alphabetical order is easy to achieve. Just replace unordered_map by map, and that's it. Then obtaining getters and so on is simple.

However alphabetical order is not the best desired option. We are used to PO, POV, and the like, for a reason, because it is more intuitive. I want to keep this custom ordering.

For this, we can have a field structure in VectorComposite, in the same manner as we have it in HasStateBlocks (therefore Sensor, Frame, Capture and Landmark), and in Processors and Problem.

This field structure_ can be defined beforehand (i.e. in the constructor) or built incrementally as state blocks are emplaced in the VectorComposite. HasStateBlocks has a similar mechanism.

Of course the order is not "very important". It just contributes to peace of mind when viewing logs and debugging.

I think that for comparing strings the order should be clear. Otherwise we can leave it unordered and whenever it has to be compared, then it has to be sorted (this is not convenient for assertions and simple checks).

If we want to specify an special order we can, for example, first in this order POVW, afterwards in alphabetical order.

If in this issue you are mostly concerned with comparing structures, then yes, a predictable order is necessary, and this can be alphabetical. In such case I'd redesign VectorComposite as a regular map to really simplify (and also make more efficient) the getter:

StateStructure getStructure()
{
  StateStructure s = "";
  for (const auto & pair_key_vec : (*this) )
  {
    s += pair_key_vec.first;
  }
  return s;
}

This would be also beneficial, for the same reason, in HasStateBlocks.

However, this has some impact in other classes. In particular, currently Problem, Processor and HasStateBlocks already have the getStructure() and this returns the predefined order, which is not alphabetical.

For example in HasStateBlocks:

const StateStructure& getStructure() const { return structure_; }

In IsMotion:

std::string getStateStructure() { return state_structure_; }

In Problem:

StateStructure getFrameStructure() const { return frame_structure_; }

BTW this API is not very consistent across classes

Maybe we should create a class for Structure with the machinery of comparing, ordering, checking if a key is included...

My worries about StateStructure being a mere typedef of string is that it is very limited.

As structure, for example, we might want to store the sizes of each block; the local sizes; the name of the state block that it is supposed to represent (in case you want to create a state block with it using a Factory), and the like. Then, as you say, you can add methods to manipulate (union, intersection, concatenation, comparison, etc).

I thought about doing it during the big merge but I had so many things to touch that I could not think clearly about what we really needed, and I did not want to create a whole now fancy class if it was not necessary.

Example:

struct StateStructure{
   string keys;
   map<char, EigenSize> sizes;
   map<char, EigenSize> sizes_local;
   map<char, string> state_block_names;

  operator ==
  operator +
  bool in(StateStructure)
  bool contains(StateStructure)
  Structure union(Structure other)
  Structure intersect(Structure other)
  
... etc
  
}

BTW as a small temporary fix concerning the last part of #349 (comment 7414), I am going to homogenize the API for the name of the structure_ variables and the getStructure() methods. This will be pushed to devel without further discussion.

@joanvallve what do you want to compare: vectors, or structures?

For vectors, we can implement operator == in VectorComposite -- although it seems not very useful
For structures, it is tricky because I still dont know what to do with the ordering of letters:
- Implement getStructureOrdered() ?
- All structures are alphabetical?

Yes, I mean comparing structures. We can make a class Structure inheriting from string and providing this functionality?

Yes, this is an option. It is good because it does not touch any other decisions already implemented.

created merge request !389 to address this issue

mentioned in merge request !389

Bad idea. See this https://stackoverflow.com/questions/6006860/why-should-one-not-derive-from-c-std-string-class

Well, at least, read the title.

How about a plane helper function

bool stateStructureCompare(const StateStructure& s1, const StateStructure& s2);

?

OK I implemented this last option and it works

bool structcmp(const StateStructure& s1, const StateStructure& s2)
{
    if (s1.size() != s2.size()) return false;

    std::set<char> s1_chars, s2_chars;

    for (const auto &c : s1)
        s1_chars.emplace(c);
    for (const auto &c : s2)
        s2_chars.emplace(c);

    return s1_chars == s2_chars;
}

TEST(StateStructure, operator_equal)
{
    StateStructure POV("POV"), PVO("PVO"), PO("PO"), POVW("POVW");

    ASSERT_TRUE ( structcmp(POV  , PVO ) );
    ASSERT_FALSE( structcmp(POV  , PO  ) );
    ASSERT_FALSE( structcmp(PO   , POV ) );
    ASSERT_FALSE( structcmp(PO   , POVW) );
    ASSERT_FALSE( structcmp(POVW , PO  ) );

    ASSERT_TRUE ( structcmp(POVW , "PVOW" ) );
    ASSERT_TRUE ( structcmp(POVW , "WPOV" ) );
}

[ RUN      ] StateStructure.operator_equal
[       OK ] StateStructure.operator_equal (17241 ms)

There is a neat alternative that I expose here:

class StateStructure
{
private:
  std::string structure_pretty;
  std::set<char> structure;

public:
  StateStructure(const std::string& s = "")
  {
    structure_pretty = s;
    for (const auto & c : s)
      emplace(c);
  }

  void append(const char& key)
  {
      structure_pretty += key;
      structure.emplace(key);
  }
  std::string pretty() const {return structure_pretty;}
  std::set<char> get() const {return structure;}
}

From here, one can use the pretty option for ordering the elements "POV" when e.g. printing on screen, but the normal structure when doing any other kind of manipulation.

Maybe (I have to try) we can try this better:

class StateStructure : public std::set<char>
{
private:
  std::string structure_pretty;

public:
  StateStructure(const std::string& s = "")
  {
    structure_pretty = s;
    for (const auto & c : s)
      emplace(c);
  }

  void append(const char& key)
  {
      structure_pretty += key;
      emplace(key);
  }
  std::string pretty() const {return structure_pretty;}
}

Why not define a to_string method and just iterate the underlying set and build structure_pretty when you want to print it instead of keeping it updated every time you add a char. Then StateStructure is just a set with a custom to_string method.

because in pretty you want to keep the order, and in set you will have alphabetical order, which is not pretty.

The idea is not to have a pretty string to print the string itself, but to have a pretty order to print the blocks in good order, e.g. if pretty = "POVba" then,

VectorComposite state("POVba");
state = bla bla
cout << "state = " << state << endl;

will produce:

state = P:(1,0) O:(0) V:(3) b:(2) a:(0)

instead of

state = a:(0) b:(2) O:(0) P:(1,0) V:(3)

I see. What I don't understand is where will this "pretty" order be defined? Because if I understand correctly

StateStructure s1("VPO"); //Pretty order "VPO"
StateStructure s2("POV"); //Pretty order "POV"

Have different "pretty" orders right? and I guess we want both to be "POV".

yes, exactly. But this applies to VectorComposite, to HasStateBlocks, to Processors, and to Problem, not merely to StateStructure.

I find annoying that when printing vectors the blocks appear in random orders (so difficult for debugging when comparing e.g. one vector against another one, visually on the screen).

Likewise, I find it not nice to have alphabetical order when it is humans that are analyzing the results.

So the pretty structure is defined in two possible ways:

In the constructor

    VectorComposite a("POV");

As blocks are appended, in the order they are appended:

    VectorComposite a;
    a.emplace('P', pos);    
    a.emplace('O', ori);
    a.emplace('V', vel);

This is already the case with the current string implementation of StateStructure. However, VectorComposite does not have a StateStructure defined, only HasStateBlocks, Problem and Processor have it.

SO I need to add a StateStructure to VectorComposite for the couts to be pretty..

What about the following easy solution:

In Problem, Processors, HasStateBlocks and VectorComposite, have a StateStructure which is a std::string as now (I need to add it also to VectorComposite).

Then add the methods

std::set<char>      getStructure()       const;
const std::string & getPrettyStructure() const;

where:

The first can be used for comparing and for all related to things machines do. bool set::operator == already exists and works.
The second is related to human interface.

Both returned structures can be iterated normally, obtaining char elements:

for (const auto& c : V.getStructure())
   will produce: 'O' 'P' 'V' ...
for (const auto& c : V.getPrettyStructure())
   will produce: 'P' 'O' 'V' ...

But the question of where to define the pretty ordering still remains, right? We cannot expect that the user will always initialize StateStructure with "POV" for instance, or that they will do

VectorComposite a;
    a.emplace('P', pos);    
    a.emplace('O', ori);
    a.emplace('V', vel);

They might do

VectorComposite a;
    a.emplace('P', pos); 
//some code   
    a.emplace('V', vel);
//some more code
    a.emplace('O', ori);

Yes, the order is human defined, because it is only important for humans.

We can also force it in the constructor, but this seems to indicate that the structure in non mutable, which is not true -- you can add blocks to a structure at any time.

This we do in Problem and in Frames, for example.

Processors can add blocks to Frames. In such case the final order depends on the sequence followed. If the Frame had a predefined structure, this will not be altered. Only blocks that were not in the predefined structure will be appended.

Logically, when a Processor appends blocks to a Frame, it does it in its own 'pretty' order. That means:

Processor 1: "POV"
Processor 2: "CDLOB"

resulting structure in Frames and Problem: "POVCDLB"

In this case, to me, "P", "O" and "V" are no longer mere chars and are another type that accepts a particular ordering, namely P < O < V. What I understand is that this ordering is defined locally for each StateStructure so it is the responsibility of the programmer to always input the values in the right order if they expect to get the same prints. Suppose we run the following code

VectorComposite a;
    a.emplace('P', pos);    
    a.emplace('O', ori);
    a.emplace('V', vel);

VectorComposite b;
    b.emplace('V', vel);
    b.emplace('O', ori);
    b.emplace('P', pos);

Now when you want to debug and do

pretty_print(a);
pretty_print(b);

I guess you would expect to see

a = P:(1,0) O:(0) V:(3)
b = P:(1,0) O:(0) V:(3)

instead you get

a = P:(1,0) O:(0) V:(3)
b = V:(3) O:(0) P:(1,0)

because you, as a programmer, have not been "careful".

What I am trying to get at, is that I feel this opens up the door for some weird cases of unexpected behavior regarding this ordering thing and I don't know if this is something we are ok with.

I am ok with it, just throwing my thoughts out here.

Following your last example, what happens when you have

Processor 1: "POV"
Processor 2: "VOP"

Whose order ends up "winning"? I guess we get Problem: POV?

Yes, all what you say is correct.

And there might be issues if programmers are not careful. However, these issues have no impact on the machine, only on humans reading the screen. This is why we are tolerating them more or less.

I think I proposed to establish an order we like: POV + alphabetical, for instance. We could use this and sort the structure according to this.

VectorComposite::getStructure() sorted string

Designs

Child items ...

Activity