romainmartinez / pyomeca-refactor

Python toolbox for biomechanics analysis
https://pyomeca.github.io/
Apache License 2.0
0 stars 1 forks source link

"meca" accessor debate #15

Open romainmartinez opened 4 years ago

romainmartinez commented 4 years ago

Le .meca est nécessaire à chaque fois? N'y a-t-il pas moyen d'intercepter le signal frontend quand on subclass?

Oui il est nécessaire pour distinguer les (très) nombreuses fonctions de xarray qui nous sont utiles et les fonctions de pyomeca. voir ici

En un coup d'oeil, je peux distinguer ce qui est implémenté dans xarray (sans le meca) et dans pyomeca.

Le pipeline EMG est un cas un peu extrême, dans le sens où c'est rare qu'on utilise autant de fonctions de pyomeca dans un appel de fonction.

romainmartinez commented 4 years ago

Je comprends la "bonne pratique" xarray, mais ça, c'est du point de vue programmeur de pyomeca. Pour un utilisateur lambda (qui ne connait pas le core de pyomeca ni même xarray), comment il sait qu'il est sensé utiliser "center" ou "abs" de meca et non de xarray et inversement le "plot" de xarray mais pas de meca? Ceci est très interne à pyomeca et c'est essentiellement les programmeurs qui savent que "abs" pour l'analog ne marchera pas avec xarray (ou peut-être que oui?)... ?

Je ne suis pas sûr que le namespace "meca" réduit l'expérience utilisateur pour plusieurs raisons.

  1. C'est la façon de travailler avec xarray (c.f. les nombreux autres projets scientifiques) qui devient standard pour travailler avec des données multidimensionnelles (2,500 packages l'utilisent) et qui est maintenu par Google et Numfocus.

  2. L'approche alternative est d'hériter de DataArray, un peu comme on avait fait dans l'ancien Pyomeca. Je ne suis pas vraiment fan de cette approche:

    • Il n'y a qu'un seul namespace mais il est complément surchargé. Dans l'ancienne version de pyomeca, un Analogs3d a 225 méthodes (!) avec len(dir(analogs)). En tant qu'utilisateur, je serais un peu perdu. Dans le nouveau pyomeca, le tab completion array.<tab> te montre toutes les fonctions xarray (132 fonctions) et array.meca.<tab> toutes les fonctions pyomeca (21 fonctions). C'est tout ce que tu as besoin de savoir en tant qu'utilisateur. Les deux namespaces sont correctement séparés et documentés.

    • Il n'y a pas de moyen direct de savoir d'où la méthode vient. Par exemple on réimplémente mean, mais dans le code c'est dur de savoir si c'est le mean de numpy ou pyomeca. Dans le nouveau pyomeca, si quelqu'un d'extérieur lit ton code, il sait directement quelles fonctions appartiennent à pyomeca et xarray.

Est-ce qu'il y a des cas où on ne met pas "meca" (autre que plot)

xarray nous permet d'éviter de ré-implémenter beaucoup de fonctions parce qu'ils en supportent déjà beaucoup (computation, interpolation, groupby) et parce que c'est label-based. Par exemple, dans l'ancien pyomeca on avait besoin de réimplémenter mean mais avec le nouveau on peut tout simplement faire markers.mean("time"), qui est — selon moi — plus descriptif que juste markers.mean() ou pire markers.mean(axis=2). Il n'y a plus besoin de contraindre la shape à être 3 dimensions et de retenir l'ordre. Plus besoin d'ajouter des dimensions ou faire des reshape, fonctions qui étaient souvent nécessaires dans pyomeca. Donc oui, pour répondre à ta question, il y aura beaucoup de cas où l'on fera appel aux fonctions xarray (sans le meca). D'où le besoin de séparer les deux namespaces.

Bref, je comprends ton point mais je trouve que l'ancien Pyomeca était pire niveau user friendly :smiley: . L'héritage de classe est nice mais dangereux avec des classes si complexes et standards que des numpy array ou des xarray DataArray.

romainmartinez commented 4 years ago

@pariterre

pariterre commented 4 years ago

Par exemple, dans l'ancien pyomeca on avait besoin de réimplémenter mean mais avec le nouveau on peut tout simplement faire markers.mean("time"), qui est — selon moi — plus descriptif que juste markers.mean() ou pire markers.mean(axis=2)

Je suis d'accord avec toi sur ce point. Avoir une interface qui ne reçoit pas tout ce que xarray possède est une excellente chose (et clairement un problème de l'ancienne méthode qu'on avait utilisé)

Dans le nouveau pyomeca, le tab completion array. te montre toutes les fonctions xarray (132 fonctions) et array.meca. toutes les fonctions pyomeca (21 fonctions).

Je suis aussi d'accord avec cela.

Je pense que ce qui me dérange en fait c'est le fait que spontanément les 132 fonctions soient accessibles. Dans le sens où en tant qu'utilisateur, je ne veux qu'avoir accès à ce qui est permis techniquement. Concernant ces 132 fonctions (- 21 = 111?) qui ne sont pas implémentées dans pyomeca, il y a probablement une raison (p.e. ceci n'a pas de sens en terme de Analog, ou on n'a tout simplement pas eu le temps en tant que programmeur), mais dans tous les cas, ceci n'a pas été validé pour utilisation dans pyomeca. Dans cette optique .meca. est une bonne idée car elle segmente avec xarray, mais (et là c'est peut-être mon background C++ qui parle), j'aurais aimé que ça aille encore plus loin en ne permettant pas d'accès aux fonctions de xarray derrière (ou en le permettant en forçant le tag). Un peu comme si c'était exactement ce que tu as fait, mais à l'envers (soit pas de namespace pour les fonctions normales et un namespace .xarray pour l'appel vers des fonctions plus profondes

Ceci dit, peut-être que techniquement ce n'est juste pas possible...

romainmartinez commented 4 years ago

Je pense que ce qui me dérange en fait c'est le fait que spontanément les 132 fonctions soient accessibles. Dans le sens où en tant qu'utilisateur, je ne veux qu'avoir accès à ce qui est permis techniquement. Concernant ces 132 fonctions (- 21 = 111?) qui ne sont pas implémentées dans pyomeca, il y a probablement une raison (p.e. ceci n'a pas de sens en terme de Analog, ou on n'a tout simplement pas eu le temps en tant que programmeur), mais dans tous les cas, ceci n'a pas été validé pour utilisation dans pyomeca.

En fait, je ne vois pas de cas où une fonction de xarray n'est pas capatible avec pyomeca et inversement. Le workflow que je vois est le suivant. 80% du temps, tu vas manipuler tes données, créer des graphiques et appliquer des fonctions directement avec l'API de xarray car les méthodes sont très généralistes grâces aux labels. Les 20% du temps où tu a besoin d'une fonctionnalité plus "biomécanique", c'est la que tu vas utiliser meca. J'ai choisi 80%-20% un peu arbitrairement, mais l'idée est que la grande majorité du temps tu utilises juste xarray et que pyomeca est juste un addon quand tu as besoin de quelque chose plus specialisé.

pariterre commented 4 years ago

D'accord, donc si on reprend ton exemple de EMG, et c'est peut-être ça qui m'a confondu (puisque je n'ai pas encore vu le code), quelle est la raison pour laquelle "abs" a été réimplémentée? Mon interprétation était que si une fonction aussi de base avait besoin de l'être, alors nous n'utiliserions en réalité à peu près jamais la base.

Dernière question! ;) Si je suis un utilisateur lambda, mon workflow pour trouver une fonction est de rechercher si celle-ci apparait en premier dans meca avant normal (pas meca...)? Qu'arrive-t-il avec les fonctions qui ont déjà été réimplémentées? Quelles étaient les raisons pour cela? Mon interprétation était, par exemple, que les dimensions devaient être réajustées (un peu comme axis dans numpy)

romainmartinez commented 4 years ago

D'accord, donc si on reprend ton exemple de EMG, et c'est peut-être ça qui m'a confondu (puisque je n'ai pas encore vu le code), quelle est la raison pour laquelle "abs" a été réimplémentée?

Comme dans l'ancien pyomeca, tout simplement parce qu'il n'y a pas de fonction abs disponible en méthode chaining ; tu dois faire np.abs(array).

Si je suis un utilisateur lambda, mon workflow pour trouver une fonction est de rechercher si celle-ci apparait en premier dans meca avant normal (pas meca...)?

Le workflow est de seulement chercher les fonctions "biomécaniques" et plus high-level dans meca.

Qu'arrive-t-il avec les fonctions qui ont déjà été réimplémentées? Quelles étaient les raisons pour cela? Mon interprétation était, par exemple, que les dimensions devaient être réajustées (un peu comme axis dans numpy)

J'ai enlevé les fonctions qu'on avait réimplémentées qui ne sont plus nécessaires. Voici une sélection, avec le code xarray qui les remplace:

Et d'autres fonctions reliées à la logique de subclass (__parse_item__, __getitem__, __setitem__, __arary_finalize__, self.dynamic_child_cast, __iter__, __next__, _get_class_name, update_misc)

pariterre commented 4 years ago

Parfait, je pense que j'adhère à l'idée

Qu'arrive-t-il avec les fonctions qui ont déjà été réimplémentées? Quelles étaient les raisons pour cela?

Je voulais dire les 21 fonctions, est-ce qu'elles sont toutes des implémentations ou c'est des surcharges des fonctions xarray? Si c'est des surcharges, qu'elles étaient les raisons de surcharger

Merci pour les réponses!