VectorComposite::getStructure() sorted string

To easily check, compare, etc. VectorComposite should have the method getStructure() however, the ordering in the unordered_map and the string provided by the user does necessarily follow the same order.

So I propose to return a sorted (alphabetically) string. And agreeing on the Wolf::Structure are a sorted, maybe a class instead of a typedef is required here, I don't know.

Opinions? @jsola

added Agreement required label

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".