[RTR] show_labels (markers, CoM, muscles, ligaments, contacts, ...)

[EveCharbie]

This change is 

[EveCharbie]

@Ipuch when you have time, I think this is ready for review :)

[Ipuch]

Reviewed 5 of 7 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @EveCharbie)

---

-- commits line 21 at r2:
Don't hesitate to squash your commits to avoid such single lines commits.

Code quote:

- e16268b: woups

- 0e92446: blacked

- a4c6310: woups

---

pyorerun/xp_components/markers.py line 102 at r2 (raw file):

                rr.components.TextBatch(markers_names).partition(partition),
            ]
        }

Show label should appear also here

Code quote:

        return {
            self.name: [
                rr.Points3D.indicator(),
                rr.components.Position3DBatch(flattened_markers).partition(partition),
                rr.components.ColorBatch([self.markers_properties.color for _ in range(self.nb_frames)]),
                rr.components.RadiusBatch([self.markers_properties.radius for _ in range(self.nb_frames)]),
                rr.components.TextBatch(markers_names).partition(partition),
            ]
        }

---

pyorerun/model_phase.py line 52 at r2 (raw file):

    def to_rerun_links(self, frame: int):
        """Update the links between markers and models"""
        for i, rr_link in enumerate(self._rerun_links_without_none):

Why is it necessary ? Is _rerun_links_without_none broken ? I would repair this one first

---

pyorerun/model_components/model_markers.py line 60 at r2 (raw file):

                rr.components.RadiusBatch([self.marker_properties.radius for _ in range(nb_frames)]),
                rr.components.TextBatch(markers_names).partition(partition),
                rr.components.ShowLabelsBatch([False for _ in range(nb_frames)]),

please update this line

Code quote:

  rr.components.ShowLabelsBatch([False for _ in range(nb_frames)]),

---

pyorerun/phase_rerun.py line 130 at r2 (raw file):

                    :, model.marker_names.index(marker), :
                ]
            tracked_markers = PyoMarkers(reordered_markers, channels=list(model.marker_names))

Thanks ! move the piece of code to a utils.py :) and add an if statement in case the number of markers is not the same etc...

Code quote:

            reordered_markers = np.zeros_like(tracked_markers.to_numpy())
            for marker in model.marker_names:
                marker_index = tracked_marker_names.index(marker)
                reordered_markers[:, marker_index, :] = tracked_markers.to_numpy()[
                    :, model.marker_names.index(marker), :
                ]
            tracked_markers = PyoMarkers(reordered_markers, channels=list(model.marker_names))

---

pyorerun/phase_rerun.py line 136 at r2 (raw file):

        )

    def add_xp_markers(self, name, markers: PyoMarkers, show_tracked_marker_labels: bool = True) -> None:

I don't like the fact we need to add a arg, the show label info should move to Pyomarkers. I have to accept it this way first, I understand.

Code quote:

show_tracked_marker_labels: bool = True

---

pyorerun/phase_rerun.py line 105 at r2 (raw file):

            )

    def __add_tracked_markers(self, model: AbstractModel, tracked_markers: PyoMarkers) -> None:

I think you blacked l80 ?

---

pyorerun/abstract/markers.py line 62 at r2 (raw file):

        self.radius = radius
        self.color = color
        self.show_labels = show_labels

It may require a list / array of bool, if so add a method 'show_labels_to_rerun()'

Code quote:

self.show_labels = show_labels

[EveCharbie]

On s'appelle et on déjeune ;)

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Ipuch)

---

-- commits line 21 at r2:

Previously, Ipuch (Pierre Puchaud) wrote…

Don't hesitate to squash your commits to avoid such single lines commits.

Why ? I like to isolate "black" commit as I know there was no logic done in those commits.

---

pyorerun/model_phase.py line 52 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Why is it necessary ? Is _rerun_links_without_none broken ? I would repair this one first

Could you explain what "rerun_links_without_none" is supposed to do ?
In my case, I did:

viz = pyorerun.PhaseRerun(t)
viz.add_animated_model(model, q)
viz.add_animated_model(model, q, tracked_markers=pyomarkers)

Since the second model has experimental markers (rerun_links), and the first one doesn't the loop threw an error.

---

pyorerun/phase_rerun.py line 105 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

I think you blacked l80 ?

I am on black=25.1.0 (the latest on conda-forge) and I blacked black . -l120

---

pyorerun/phase_rerun.py line 130 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Thanks ! move the piece of code to a utils.py :) and add an if statement in case the number of markers is not the same etc...

Moved it to utils/markers_utils.py
There was already a check for the marker size

---

pyorerun/phase_rerun.py line 136 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

I don't like the fact we need to add a arg, the show label info should move to Pyomarkers. I have to accept it this way first, I understand.

I agree. I think we would have more flexibility if Pyomarkers was reimplemented instead of taken from pyomeca.

---

pyorerun/abstract/markers.py line 62 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

It may require a list / array of bool, if so add a method 'show_labels_to_rerun()'

I would like to talk about this one (currently: bool works, but we do not have the possibility to show only some labels).

---

pyorerun/model_components/model_markers.py line 60 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

please update this line

Done.
Thanks, missed it !

---

pyorerun/xp_components/markers.py line 102 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

Show label should appear also here

I did the modification, but I am coding blindly :/ Could you help me figure out which test/example uses to_chunk so that I can test it ?

[Ipuch]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EveCharbie)

---

pyorerun/xp_components/markers.py line 102 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

I did the modification, but I am coding blindly :/ Could you help me figure out which test/example uses to_chunk so that I can test it ?

.rerun() use chunks and rerun_by_frame(), use the other mode.

---

pyorerun/utils/markers_utils.py line 19 at r3 (raw file):

            f"They must have the same names and shape.\n"
            f"Current markers are {model.marker_names} and\n tracked markers: {tracked_markers.channel.data.tolist()}."
        )

We should have an extra check beforehand to ensure all the names are in both lists, if not it crashes

Code quote:

    if shape_of_markers_is_not_consistent:
        raise ValueError(
            f"The markers of the model and the tracked markers are inconsistent. "
            f"They must have the same names and shape.\n"
            f"Current markers are {model.marker_names} and\n tracked markers: {tracked_markers.channel.data.tolist()}."
        )

[Ipuch]

Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @EveCharbie)

---

pyorerun/model_phase.py line 52 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

Could you explain what "rerun_links_without_none" is supposed to do ?
In my case, I did:

viz = pyorerun.PhaseRerun(t)
viz.add_animated_model(model, q)
viz.add_animated_model(model, q, tracked_markers=pyomarkers)

Since the second model has experimental markers (rerun_links), and the first one doesn't the loop threw an error.

idx_not_none = [i for i, rr_link in enumerate(self.rerun_links) if rr_link is not None]

for i, rr_link in zip(idx_not_none, self._rerun_links_without_none):
rr_link.to_rerun(self.q[i][:, frame], self.tracked_markers[i][:, :, frame])

---

pyorerun/abstract/markers.py line 62 at r2 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

I would like to talk about this one (currently: bool works, but we do not have the possibility to show only some labels).

to_rerun()

[EveCharbie]

Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @Ipuch)

---

pyorerun/model_phase.py line 52 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

idx_not_none = [i for i, rr_link in enumerate(self.rerun_links) if rr_link is not None]

for i, rr_link in zip(idx_not_none, self._rerun_links_without_none):
rr_link.to_rerun(self.q[i][:, frame], self.tracked_markers[i][:, :, frame])

Done.

---

pyorerun/abstract/markers.py line 62 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

to_rerun()

Done.

---

pyorerun/utils/markers_utils.py line 19 at r3 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

We should have an extra check beforehand to ensure all the names are in both lists, if not it crashes

Done.

---

pyorerun/xp_components/markers.py line 102 at r2 (raw file):

Previously, Ipuch (Pierre Puchaud) wrote…

.rerun() use chunks and rerun_by_frame(), use the other mode.

Done.

[qlty-cloud-legacy]

The PR diff size of 39135 lines exceeds the maximum allowed for the inline comments feature.

[EveCharbie]

Also fixed the gait example meshes (issue #58).

[Ipuch]

Reviewed 1 of 1 files at r4, 10 of 10 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EveCharbie)

---

pyorerun/utils/markers_utils.py line 19 at r3 (raw file):

Previously, EveCharbie (Eve Charbonneau) wrote…

Done.

Not yet. I except a if case like this beforehand:

if tracked_marker_names_are_not_all_in_model_markers:
        ...
        raise ValueError(f"Missing  {xx} in tracked_markers")

[Ipuch]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @EveCharbie)

[qlty-cloud-legacy]

Code Climate has analyzed commit e614d0f and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.