Implemented draft of RMMR algorithm. #41

RamSaw · 2018-03-28T23:18:35Z

First draft of an RMMR algotrithm. Algorithm is based on calculating distance between a method and a class where it can be moved. Distance between the method and the class is mean of distances between the method and methods in the class. Distance between two methods is calculated by the following formula: 1 - size_of_intersection_of_conceptual_sets / size_of_union_of_conceptual_sets. Conceptual set consists of classes (only classes that are in specified scope) whose methods are called in the body of given method and classes of method arguments.

In code the following was done. I created my classes for searching entities which my algorithm needs. They are RmmrStrategy and RmmrEntitySearcher. Implementation is very similar to the ones that are implemented already (NewStrategy and EntitySearcher). The difference is that my implementation do not consider any fields and additionally processes new statements in java language. Also I added RMMR algorithm implementation. As a basis I took ARI algorithm implementation so they must very be similar.

I added switch in RefactoringExecutionContext to run different entity searchers for RMMR and others algorithms. And changed RefactoringDetectionTest: it ran all algos but assertions do not fit for RMMR.

Further actions are: test algorithm (I began it and added some test classes) and check results with these that are described in the article, think about the best accuracy formula.

ujohnny · 2018-04-11T10:22:10Z

Добрый день,

Давайте напишем описание для пул реквеста, как мини-документацию. Со ссылкой на документ, описывающий алгоритм, чтобы можно было в будущем (и текущем) к нему обратиться и понять, что здесь сделано верно, а что нет.

…eak and wrong variable name (attribute instead of attributeType). Also added several test cases and changed accuracy formula to be more accurate.

…re not correct (it doesn't supports fields), so I added exact list of algos to run.

ujohnny · 2018-04-12T12:07:48Z

src/main/java/org/ml_methods_group/algorithm/RMMR.java

+public class RMMR extends Algorithm {
+    public static final String NAME = "RMMR";
+
+    private static final Logger LOGGER = Logging.getLogger(MRI.class);


ujohnny · 2018-04-12T12:15:45Z

src/main/java/org/ml_methods_group/algorithm/RMMR.java

+    private double getDistance(MethodEntity methodEntity1, MethodEntity methodEntity2) {
+        // TODO: Maybe add to methodEntity2 source class where it is located?
+        int sizeOfIntersection = intersection(methodEntity1.getRelevantProperties().getClasses(),
+                methodEntity2.getRelevantProperties().getClasses()).size();


You might like to extract 2 variables there and these 4 lines will become more readable

ujohnny · 2018-04-12T12:16:58Z

src/main/java/org/ml_methods_group/algorithm/RMMR.java

+                methodEntity2.getRelevantProperties().getClasses()).size();
+        int sizeOfUnion = union(methodEntity1.getRelevantProperties().getClasses(),
+                methodEntity2.getRelevantProperties().getClasses()).size();
+        return sizeOfIntersection == 0 ? 1 : 1 - (double) sizeOfIntersection / sizeOfUnion;


don't hesitate to use parenthesis around conditional statements

If I understood you correctly, you suggested to write so: (sizeOfIntersection == 0). If yes, then I've corrected it.

ujohnny · 2018-04-12T12:19:49Z

src/main/java/org/ml_methods_group/algorithm/entity/RmmrEntitySearcher.java

+            String className = null;
+            PsiType type = expression.getType();
+            if (type instanceof PsiClassType) {
+                className = ((PsiClassType) expression.getType()).getClassName();


(PsiClassType) type

ujohnny · 2018-04-12T12:42:59Z

src/main/java/org/ml_methods_group/algorithm/RMMR.java

+        progressCount.set(0);
+
+        entities.getMethods().forEach(methodEntity -> {
+            List<ClassEntity> methodClass = entities.getClasses().stream()


entities -> List of Entity
classEntities -> List of ClassEntity
methodClass -> ???

Renamed to methodClassEntity. This list must be size of 1 and consist of class entity in which method is located.

ujohnny · 2018-04-12T13:01:28Z

src/main/java/org/ml_methods_group/algorithm/RMMR.java

+        }
+        final String targetClassName = targetClass.getName();
+        double accuracy = (1 - minDistance) * difference;  // TODO: Maybe consider amount of entities?
+        if (!targetClassName.equals(entity.getClassName())) {


that's a bit odd check looking on the code. if CLASS whose method is being analyzed there is taken into account when calculating the distance -- algorithm above might give a closer metric and you'll lose correct result. Probably more correct approach is to perform this check before the call to getDistance. Please correct me if I'm wrong.

No, it seems to me to be correct. We measure distance between CLASS and method, and distances between other classes and method. getDistance considers if it is calculating distance between CLASS and method so it will be counted as described in the article (without measuring method with itself which would be always 0 distance). And we suggest refactoring if there is less distance than with CLASS.

ujohnny · 2018-04-13T11:07:31Z

src/main/java/org/ml_methods_group/algorithm/entity/RmmrEntitySearcher.java

+        public void visitFile(PsiFile file) {
+            indicator.checkCanceled();
+            if (strategy.acceptFile(file)) {
+                LOGGER.info("Indexing " + file.getName());


doesn't look consistent to methods below

as well as info level for file visits is too high.

Made consistent. Not sure that I understood correctly about too high level. Does it mean that this information is not so important to be logged in info? I peeped this code in EntitySearcher implementation, it is exactly like there. If it is really too high level then we need to correct it in EntitySearcher also.

ujohnny · 2018-04-13T11:14:39Z

src/main/java/org/ml_methods_group/algorithm/entity/RmmrEntitySearcher.java

+        LOGGER.info("Properties calculated");
+        LOGGER.info("Generated " + classes.size() + " class entities");
+        LOGGER.info("Generated " + methods.size() + " method entities");
+        LOGGER.info("Generated " + 0 + " field entities. Fields are not supported.");


does this line make any sense?

I don't know. I report that this algorithm does not work with fields. I think it serves for consistency, because log information in EntitySearcher is structured the same.

…bugging that is why single thread version is commented not deleted.

… it is called by class name but IDEA doesn't considers it.

ujohnny · 2018-05-21T16:00:57Z

src/main/java/org/ml_methods_group/algorithm/RMMR.java

+        double differenceWithSourceClassCoefficient = (1 - minDistance) * differenceWithSourceClass;
+        double accuracy = (0.7 * differenceWithSourceClassCoefficient + 0.3 * difference) *
+                sourceClassCoefficient * targetClassCoefficient;
+        // accuracy = 1;
        if (accuracy >= 0.01 && !targetClassName.equals(entity.getClassName())) {


accuracy >= 0.01
is it just a comparison with epsilon?

Moved to final static constant.

…d accuracy formula, added annotations @NotNull.

…mize and rearrange code, fix todo in code and add stack for classes.

…d stack for nested classes entity searching. Also changed ArrayList<Double> to double[] to avoid boxing and unboxing, so now calculation runs in acceptable time.

…o speed up calculations.

…nces on collections returned by method values() of the map. It is backed by the map, so it is more optimized in terms of memory than previous version.

…mming/ArchitectureReloaded into RMMR_implementation Because PorterStemmer.jar was added externally.

…"as", "i" (as variable in for loop) by length, now accept only with length > 2.

…factorings.

…us vector in one entity became small and memory usage became small, this accelerated algorithm significantly.

…gic moved to PropertiesCalculator, so now we walk through PSI tree only two times, not three times.

…thod test started pass.

…t so different, but from my point of view a little bit better.

…: it tightly depends on RMMR configuration.

…ySearcherTest. Reason: it tightly depends on RMMR configuration.

…: ClassA.methodFromClassA()

…acy mostly was lower than 0.7 and accuracy between 0.5 and 0.7 was giving pretty reasonable refactorings. But other algorithms have normal accuracy

# Conflicts: # src/main/java/org/jetbrains/research/groups/ml_methods/algorithm/entity/OldEntity.java # src/main/java/org/jetbrains/research/groups/ml_methods/refactoring/RefactoringExecutionContext.java # src/test/java/org/jetbrains/research/groups/ml_methods/algorithm/AlgorithmAbstractTest.java # src/test/java/org/jetbrains/research/groups/ml_methods/algorithm/AriTest.java # src/test/java/org/jetbrains/research/groups/ml_methods/algorithm/CcdaTest.java # src/test/java/org/jetbrains/research/groups/ml_methods/algorithm/HacTest.java Moved bag and contextualVector to RelevantProperties. Architecture design must be reconsidered.

Implemented draft of RMMR algorithm.

174fcd2

RamSaw added 2 commits April 11, 2018 22:04

Made several changes: corrected wrong behaviour because of missing br…

6f61018

…eak and wrong variable name (attribute instead of attributeType). Also added several test cases and changed accuracy formula to be more accurate.

Corrected test: it ran all algorithms, but assertions for RMMR algo a…

0fd31dd

…re not correct (it doesn't supports fields), so I added exact list of algos to run.

ujohnny reviewed Apr 13, 2018

View reviewed changes

RamSaw added 6 commits April 15, 2018 13:20

Corrected marks from pull request.

bc85cad

Added tests for MovieRentalStore example.

df7b1e0

Added javadocs and made code clean up.

08536b2

Changed to run parallel. Parallel executing is not comfortable for de…

538ff85

…bugging that is why single thread version is commented not deleted.

Made RMMR constructor public. IDEA says it can be package-private but…

59b19a2

… it is called by class name but IDEA doesn't considers it.

Made RmmrEntitySearcher more consistent and turned to qualified names.

1fce0bc

ujohnny reviewed May 21, 2018

View reviewed changes

RamSaw and others added 16 commits May 21, 2018 21:37

Moved 0.01 hardcoded constant to static final constant.

79ebe6b

Removed new expressions, added Builders, Utils, Factory cases, change…

22ac2b6

…d accuracy formula, added annotations @NotNull.

Added contextual similarity calculation. Very raw version. TODO: opti…

e227e2b

…mize and rearrange code, fix todo in code and add stack for classes.

Fixed null pointer exception by change get to getOrDefault. Also adde…

2d0ddd0

…d stack for nested classes entity searching. Also changed ArrayList<Double> to double[] to avoid boxing and unboxing, so now calculation runs in acceptable time.

Changed to multisets and all maps from entity moved to entity class t…

e9f71fd

…o speed up calculations.

Deleted redundant documents sustaining. Now it consists of two refere…

00333f5

…nces on collections returned by method values() of the map. It is backed by the map, so it is more optimized in terms of memory than previous version.

Added support of configuration of algorithm.

2c4d416

Added stemming.

c1379f4

Reverted ArchitectureReloaded.iml file.

2400fb5

fix conflicts commit

66c2b2c

Added jar for stemming

826ee53

Removed qualified name error fix because tests were not passing.

83168b2

Merge branch 'RMMR_implementation' of https://github.com/ml-in-progra…

9eef351

…mming/ArchitectureReloaded into RMMR_implementation Because PorterStemmer.jar was added externally.

Erased bug with qualified names and added excluding short names like …

42a7471

…"as", "i" (as variable in for loop) by length, now accept only with length > 2.

Added tests for RMMR and added support for setting up or not field re…

08740ec

…factorings.

Speeded up algorithm by adding coordinates that are only not null. Th…

ebe7152

…us vector in one entity became small and memory usage became small, this accelerated algorithm significantly.

RamSaw added 18 commits July 10, 2018 12:20

Fixed tests. Failing test were uncommented by mistake.

b9e8966

Algorithm was optimized more, now bag finder class deleted and its lo…

28153b7

…gic moved to PropertiesCalculator, so now we walk through PSI tree only two times, not three times.

Found mistake by dividing on zero. Fixed and as a consequence move me…

d2f1015

…thod test started pass.

RMMR was set up to pass more test cases. On real project output is no…

c0503ef

…t so different, but from my point of view a little bit better.

Ignored tests in RmmrDistancesTest and RmmrEntitySearcherTest. Reason…

9f884cd

…: it tightly depends on RMMR configuration.

Fixed tests failure. Ignored tests in RmmrDistancesTest and RmmrEntit…

5cd9df9

…ySearcherTest. Reason: it tightly depends on RMMR configuration.

Added support for method references: ClassA::methodInClassA.

82ade8e

Added support for class references. For example call of static method…

bc238ca

…: ClassA.methodFromClassA()

Added failure explanations for all tests which fail.

a342283

Corrected weights for conceptual and contextual similarity.

f15f724

Corrected accuracy formula - now output is a little bit better.

9bd97d4

Made clean up of code, code prepared for review. Deleted old test data.

fd94e63

Corrected accuracy formula to fit other algorithms. Before that accur…

3d86756

…acy mostly was lower than 0.7 and accuracy between 0.5 and 0.7 was giving pretty reasonable refactorings. But other algorithms have normal accuracy

Fixes #50

afa2096

Fixed test failure.

9fc5780

Reverted bug fix, because it introduced new bug.

ac4cf63

Implemented draft of RMMR algorithm. #41

Are you sure you want to change the base?

Implemented draft of RMMR algorithm. #41

Uh oh!

Conversation

RamSaw commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ujohnny commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RamSaw commented Mar 28, 2018 •

edited

Loading