Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue6 #25

Closed
wants to merge 7 commits into from
Closed

Issue6 #25

wants to merge 7 commits into from

Conversation

tfrancart
Copy link
Collaborator

@tfrancart tfrancart commented Mar 30, 2017

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
Copy link

codecov-io commented Mar 30, 2017

Codecov Report

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

Impacted file tree graph

@@            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%> (ø) ⬆️
...ain/java/fr/onagui/control/AlignmentControler.java 0% <0%> (ø) ⬆️
src/main/java/fr/onagui/gui/AlignmentGUI.java 0% <0%> (ø) ⬆️
src/main/java/fr/onagui/gui/TypeEditor.java 0% <0%> (ø)
src/main/java/fr/onagui/gui/ValidityEditor.java 0% <0%> (ø) ⬆️
src/main/java/fr/onagui/alignment/Mapping.java 33.51% <0%> (-4.22%) ⬇️
src/main/java/fr/onagui/alignment/io/SkosImpl.java 22.22% <0%> (-45.35%) ⬇️
...in/java/fr/onagui/alignment/io/EuzenatRDFImpl.java 66.82% <0%> (-10.96%) ⬇️
...a/fr/onagui/alignment/container/SKOSContainer.java 52.85% <0%> (-0.94%) ⬇️
... 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
Copy link
Owner

lmazuel commented Apr 2, 2017

@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
Copy link
Collaborator Author

  • 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
Copy link
Owner

lmazuel commented Apr 3, 2017

@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 :)

…u changement du changement de type et de validity par un remove/add(solution 2)
@LYNCETECH
Copy link
Contributor

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
Copy link
Owner

lmazuel commented Apr 5, 2017

@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
Copy link
Collaborator Author

tfrancart commented Apr 6, 2017 via email

@lmazuel
Copy link
Owner

lmazuel commented Apr 12, 2017

@tfrancart @LYNCETECH je suis perdu là, la feature branch #41 c'était justement par pour cette PR ici? Dans ce cas pourquoi elle s'appelle "Issue 15", et ici "Issue 6"?

@tfrancart
Copy link
Collaborator Author

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

@tfrancart tfrancart mentioned this pull request Apr 12, 2017
@tfrancart
Copy link
Collaborator Author

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

@lmazuel
Copy link
Owner

lmazuel commented Apr 18, 2017

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 lmazuel closed this Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants