tfrancart commented 7 years ago

Pouvoir éditer le type de la correspondance dans le tableau (soit par clic, soit dans le menu clic-droit pour des changements en masse) Fix #6

codecov-io commented 7 years ago

Codecov Report

Merging #25 into master will decrease coverage by 2.03%. The diff coverage is 0%.

@@            Coverage Diff            @@
##           master     #25      +/-   ##
=========================================
- Coverage   20.24%   18.2%   -2.04%     
=========================================
  Files          47      49       +2     
  Lines        3913    4115     +202     
  Branches      527     565      +38     
=========================================
- Hits          792     749      -43     
- Misses       3059    3307     +248     
+ Partials       62      59       -3

Impacted Files	Coverage Δ
src/main/java/fr/onagui/gui/TypeRenderer.java	`0% <0%> (ø)`
src/main/java/fr/onagui/gui/MappingTableModel.java	`0% <0%> (ø)`	:arrow_up:
...ain/java/fr/onagui/control/AlignmentControler.java	`0% <0%> (ø)`	:arrow_up:
src/main/java/fr/onagui/gui/AlignmentGUI.java	`0% <0%> (ø)`	:arrow_up:
src/main/java/fr/onagui/gui/TypeEditor.java	`0% <0%> (ø)`
src/main/java/fr/onagui/gui/ValidityEditor.java	`0% <0%> (ø)`	:arrow_up:
src/main/java/fr/onagui/alignment/Mapping.java	`33.51% <0%> (-4.22%)`	:arrow_down:
src/main/java/fr/onagui/alignment/io/SkosImpl.java	`22.22% <0%> (-45.35%)`	:arrow_down:
...in/java/fr/onagui/alignment/io/EuzenatRDFImpl.java	`66.82% <0%> (-10.96%)`	:arrow_down:
...a/fr/onagui/alignment/container/SKOSContainer.java	`52.85% <0%> (-0.94%)`	:arrow_down:
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c94539c...26c4456. Read the comment docs.

lmazuel commented 7 years ago

@tfrancart @LYNCETECH Ca me semble bien, mais attention sur un point: Un Mapping possède une fonction "hash" et "compareTo" pour pouvoir les placer dans un SortedSet ou un HashSet. Ces fonctions ont toujours supposé que url1, url2, type et validity étaient des constantes. Autrement dit, qu'on utiliserait un nouvel object si l'un de ces quatre paramètres changeait. Je l'ai oublié moi-meme quand j'ai ajouté l'edition de validité, et ici on ajoute un cran avec l'edition de type.

Ce qui veut dire que si on edite validity/type sur un Mapping deja dans un SortedSet, le comportement est incoherent. Il y a un risque certain que l'objet soit introuvable ("contains" échoue), ou qu'on crée un bug mémoire si on édite avec les valeurs qu'un autre objet a déjà dans le SortedSet.

Le plus on change d'éléments durant une session, le plus on risque d'avoir des crash mémoire ou des comportements incohérents d'OnAGUI.

Solution 1: Réduire la comparaison/hash de deux alignements à uniquement les uris. Ca me semblaient un peu brutal: cela voudrait dire qu'on ne peut pas avoir deux mappings pour un meme couple d'uris avec deux types / validités différentes (ca peut se discuter).

Solution 2: Interdire le "setType" et "setValidity" sur l'objet Mapping pour garder la cohérence de ces méthodes. Dans ce cas, toute modification de ces champs revient à faire un "remove/create" dans l'objet Alignement pour garder les SortedSet cohérent. Si on ne veut pas le gérer dans Alignement, on peut faire une sous-classe de SortedSet qui s'auto-reorganise quand les objets changent: http://stackoverflow.com/a/8030165/4074838 http://stackoverflow.com/a/2581450/4074838

Solution 3: Arreter de considérer que des Mapping sont comparable et retirer complètement les fonctions compare/hashcode. Il faut changer tout les "SortedSet" par des "List". J'ai du mal à visualiser l'impact sur les perfs, je ne me rappelle plus exactement pourquoi je trouvais utile de pouvoir comparer les Mapping entre eux.

La solution 1 est la plus restrictive en terme d'alignement (critère d'unicité basé sur les uris uniquement) , la solution 2 risque d'avoir un impact sur les perfs quand on change un type/validité (lag de l'UI si beaucoup d'alignement chargé). La solution 3 peut etre la meilleur comme la pire, il faudrait retirer les méthodes compare/hashcode et voir exactement jusqu'ou ca tire les changements.

Vous en pensez quoi?

tfrancart commented 7 years ago

La solution 1 ne me semble pas bonne
La solution 2 me semble faisable, mais je n'ai pas d'idée sur les perfs (note : j'ai l'impression que même en changeant une seule validité aujourd'hui, l'intégralité du tableau est réaffiché ? voir l'appel de la méthode refreshGUIFromModel() depuis ValidityEditor)
La solution 3 est relativement faisable - dans le sens où elle n'est pas très impactante sur le code. Nous n'avons aucune idée de son impact sur les perfs

Pour les perfs, tu es mieux placé que nous - nous n'avons pas vraiment fait de tests de perfs sur des gros volumes. La solution 2 me semble la moins impactante si tu confirmes mon impression que l'intégralité du tableau est déjà réaffichée à chaque fois.

lmazuel commented 7 years ago

@tfrancart @LYNCETECH Solution 2 ca me va, on verra si les perfs suivent pas (autrement dit, si changer type/validité freeze l'interface). Oui la table doit se remettre à jour pour chaque ajout (comme elle le fait quand l'import ajoute des mappings). Au pire un refresh a appelé au bon endroit, mais ca devrait pas etre utile. Donc retirer "setType/setValidity" de Mapping, et changer toutes les erreurs de compilations par "remove/add" et ça devrait le faire :)

LYNCETECH commented 7 years ago

Nous avons fini de mettre en place la solution 2. Nous avons constaté que les commentaires sont aussi éditables, est ce qu'on peut apporter ces changements également sur les commentaires en interdisant le "setComment"?. Qu'en penses tu?

lmazuel commented 7 years ago

@tfrancart @LYNCETECH Bon, j'ai repris le code avec un peu de recul. Actuellement Mapping a cet ensemble d'attributs:

    private T firstConcept = null;
    private V secondConcept = null;
    private double score = 0.0;
    private MAPPING_TYPE type = MAPPING_TYPE.EQUIV;
    private String method = UNKNOW_METHOD;
    private VALIDITY validity = VALIDITY.TO_CONFIRM;
    private String comment = "";
    private Map<String, String> meta = new TreeMap<String, String>();
    private DateTime creationDate = new DateTime();

Question ouverte (oublions OnAGUI): quels sont ceux qui rentrent en compte dans l'unicité? Est-ce qu'on considère que la moindre modification en fait un object différent au sens de l'égalité?

Je vois qu'il y a un bug entre "equals" et "compareTo" (i.e. je suis capable de creer deux objets o1 et o2 tel que o1.compareTo(o2) == 0 mais o1.equals(o2) == False, ce qui est incorrect)

Si on répond a la question principale du critère d'unicité, alors je peux répondre a la question sur quoi faire du "comment" :)

tfrancart commented 7 years ago

Le 5 avril 2017 à 18:59, Laurent Mazuel notifications@github.com a écrit :

@tfrancart https://github.com/tfrancart @LYNCETECH https://github.com/LYNCETECH Bon, j'ai repris le code avec un peu de recul. Actuellement Mapping a cet ensemble d'attributs:

private T firstConcept = null; private V secondConcept = null; private double score = 0.0; private MAPPING_TYPE type = MAPPING_TYPE.EQUIV; private String method = UNKNOW_METHOD; private VALIDITY validity = VALIDITY.TO_CONFIRM; private String comment = ""; private Map<String, String> meta = new TreeMap<String, String>(); private DateTime creationDate = new DateTime();

Question ouverte (oublions OnAGUI): quels sont ceux qui rentrent en compte dans l'unicité? Est-ce qu'on considère que la moindre modification en fait un object différent au sens de l'égalité?

Pour moi :

firstConcept
secondConcept
type
method

Une même méthode d'alignement ne peut pas produire 2 alignements du même type entre les 2 mêmes concepts.

Je vois qu'il y a un bug entre "equals" et "compareTo" (i.e. je suis

capable de creer deux objets o1 et o2 tel que o1.equals(o2) mais o1.compareTo(o2) != 0, ce qui est incorrect)

Si on répond a la question principale du critère d'unicité, alors je peux répondre a la question sur quoi faire du "comment" :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lmazuel/onagui/pull/25#issuecomment-291927139, or mute the thread https://github.com/notifications/unsubscribe-auth/ACmj8Y99m7coFRww2-qmoO76MUNGqswAks5rs8hqgaJpZM4MuWXL .

--

Thomas Francart - SPARNA Web de données | Architecture de l'information | Accès aux connaissances blog : blog.sparna.fr, site : sparna.fr, linkedin : fr.linkedin.com/in/thomasfrancart tel : +33 (0)6.71.11.25.97, skype : francartthomas

lmazuel commented 7 years ago

@tfrancart @LYNCETECH je suis perdu là, la feature branch https://github.com/lmazuel/onagui/pull/41 c'était justement par pour cette PR ici? Dans ce cas pourquoi elle s'appelle "Issue 15", et ici "Issue 6"?

tfrancart commented 7 years ago

J'ai du me mélanger les pinceaux en créant la feature branch. Je vais essayer de refaire la manip.

tfrancart commented 7 years ago

J'ai créé une nouvelle feature branch : https://github.com/lmazuel/onagui/tree/issue6-feature-branch

lmazuel commented 7 years ago

Je ferme cette PR puisqu'elle serait mergé a partir de la feature branch.

@tfrancart si tu peux résoudre les conflits sur l'autre PR, comme ca la feature branch est crée pour de vrai.

lmazuel / onagui

Issue6 #25

Codecov Report