Fix report_reader to have several time the same index (node_id, compa…

[alkino]

…rtment_id)

[jdcourcol]

@alkino can this move forward ?

[jorblancoa]

LGTM!

[tomdele]

I think we still lack quite a lot of testing here.

There is no tests for the ElementReader ?
You do not test any data values in the python part also.

These need to be tested for sure.

[alkino]

test are in python for ElementReader, test in cpp are more here to help me develop.
What do you mean by data values?

[tomdele]

I think you should test the C++ side intensively also. I guess some people (from viz or elsewhere) would prefer to use the C++ version. Also testing both (python and C++) with the same expected values can detect binding problems.

In the python part, you are testing some lengths from the .data but never the outputted data themselves. There is nothing that guaranty you retrieve the correct ones.

[tomdele]

Also did you drop the idea of numpy array for the data ?

[tomdele]

Also edge cases are missing on the python side :

• empty list
• numpy array
• empty tuple
• swapping ids : [1, 2] vs [2, 1]
• the out of range times
• the minus -1 ids
  etc ...

Also I found this using elements.h5 :

rep.get(node_ids=[99999]).data                                                                                                                                                                                              
[[], [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [],  [], []]

rep.get(node_ids=[999999]).times                                                                                                                                                       
[0.0, 0.2, 0.4, 0.6000000000000001, 0.8, 1.0, 1.2, 1.4, 1.5999999999999999, 1.7999999999999998, 1.9999999999999998, 2.1999999999999997, 2.4, 2.6, 2.8000000000000003, 3.0000000000000004, 3.2000000000000006, 3.400000000000001, 3.600000000000001, 3.800000000000001]

rep.get(node_ids=[999999]).ids
[]

I am fine with this behavior but is it the one you intended ?

[mgeplf]

Two things:

1. What is the functional testing strategy for this? Should it have beeen able to catch the problem solved by this PR?
2. In the future, could we give the opportunity to the other people who had participated in the review to give their approval before merge - that way they can check if their satisfied as well.

[alkino]

I'm on it.

[alkino]

1. What is the functional testing strategy for this?  Should it have beeen able to catch the problem solved by this PR?

The problem fixed by this PR is not a bug, this is a new way to see data, so I fix test, because we misunderstood things. So the test check what we things as the good way to see things.