Remove or comment out unused code #47

kostis · 2023-10-01T10:00:16Z

It is no surprise that there is a lot of unused/unnecessary code in the code base.
Such code was due to either attempts to fix things that did not lead anywhere or copy-pasting from another file without subsequent cleanups. Unused code is confusing and also adds (unnecessary) runtime overheads. It's better to remove or comment out this code, and I've used my best judgement to choose between these two options. Perhaps this PR can be improved in this respect (e.g. by removing some code that this first attempt just comments out).

While at it, the PR also fixes some small copy-paste errors (e.g. not checking a test, but repeating a prior test), removes some lines and does some cosmetic cleanups in methods and files which were touched.

The `IOEquivalenceTest` file seems to be semi-thought and in draft status.

Also, fix a copy-paste error in a test (which now enables it).

fhowar

I started to look at this. Many changes look ok. For some I will have to think. Can we somehow split this into easy cases and ones that remain in the PR?

kostis · 2023-10-09T07:41:57Z

Can we somehow split this into easy cases and ones that remain in the PR?

We can definitely do that, perhaps by cherry-picking the commits. But the problem is to agree what is easy (or not and why). To me, all changes are "obvious improvements". The only thing that I am unsure of is whether we can go ahead and remove more code instead of just commenting it out.

fhowar · 2023-10-09T12:15:16Z

I will go through the commits and mark easy ones as a first step

fhowar · 2023-10-09T13:13:44Z

src/main/java/de/learnlib/ralib/tools/IOSimulator.java


            if (useEqTest) {
-                ce = this.eqTest.findCounterExample(hyp, null);
+                ce = this.eqTest.findCounterExample(hyp, new ArrayList<>());


This is not ok by the convention that inputs is the set of inputs to consider, this should be a subset of the input alphabet of the provided hypothesis (from LearnLib).

We use null (probably incorrectly) to say "no restrictions". If we change this, we should fix it properly and

a) either mark the parameter clearly as unused and pass null
b) pass the actual set of inputs in tests and use the parameter in implementations

I've looked a bit more on this. Here are two things I've found:

In the entire code base of ralib, we pass null for this argument. I think this shows that either this argument is not needed in practice (and LearnLib's interface has been over-engineered; perhaps its interface should only specify the 1-argument version of this method), or we do not use the functionality of this method properly.

If I am not mistaken, this particular call which is flagged in this comment refers to the definition of this function in IOEquivalenceTest.java. That argument is unused by the method; we may as well pass anything there...

I think it is over-engineered in LearnLib. @mtf90: can you suggest something here?

Just to pick up on @kostis' previous comment: both versions are problematic. The parameter is meant to determine which input symbols should be used for finding counterexamples. So null would get flagged by any null-analysis and new ArrayList<>() could (if implemented accordingly) at most return epsilon as a counterexample. The reason why this works in this case is (probably) because you only use the IOEquivalenceTest and IORandomWalk which ignore this parameter and use an internal field for the inputs.

As for the question whether this is over-engineered, I think it has its benefits. Passing the eligible input symbols as a parameter allows the equivalence oracle to not have to store the inputs. Especially for LearnLib features such as Resumable or SupportsGrowingAlphabet (which allow you to snapshot learning states and expand in different input-directions) this is quite useful because it reduces data dependencies. This is currently already a hassle with caches which do store the referenced alphabet and therefore need to be updated as well when new input symbols are introduced. By removing this parameter this management would also be necessary for equivalence oracles.

I can see your point that it adds somewhat of an overhead in the learning loop, but it also reduces overhead when implementing equivalence oracles. So for right now, I would suggest to keep the interface as is and try to simplify the equivalence oracles if possible.

fhowar

Please remove marked changes to equivalence tests for now. @mtf90 and @FredrikTaquist to comment on counterexample analyses / LearnLib design and optimised symbolic suffixes

fhowar · 2023-10-09T13:14:03Z

src/main/java/de/learnlib/ralib/tools/IOSimulator.java

@@ -328,7 +327,7 @@ public void run() {

                DefaultQuery<PSymbolInstance, Boolean> ce2 = null;

-                ce2 = (findCounterexamples ? this.randomWalk.findCounterExample(hyp, null) : ce);
+                ce2 = (findCounterexamples ? this.randomWalk.findCounterExample(hyp, new ArrayList<>()) : ce);


See related comment

fhowar · 2023-11-29T14:11:21Z

src/main/java/de/learnlib/ralib/tools/IOSimulator.java


            if (useEqTest) {
-                ce = this.eqTest.findCounterExample(hyp, null);
+                ce = this.eqTest.findCounterExample(hyp, new ArrayList<>());


I think it is over-engineered in LearnLib. @mtf90: can you suggest something here?

fhowar · 2023-11-29T14:18:48Z

src/main/java/de/learnlib/ralib/equivalence/IOEquivalenceTest.java

Please do not merge this. If I remember correctly some of the one sides checks (e.g., only using sys1reg in getNext was done under the assumption that it is sufficient to use the values in registers of the SUT for finding counterexamples). I would like to discuss this first and we should then write a short documentation that this will not check equivalence between two automata in general!

Similar comment as for RAEquiivalenceTest. I'll wait until these two cases get resolved.

I will refactor this in a separate PR

fhowar · 2023-11-29T14:21:06Z

src/main/java/de/learnlib/ralib/equivalence/RAEquivalenceTest.java

Please do not merge this. If I remember correctly some of the one sides checks (e.g., only using sys1reg in getNext was done under the assumption that it is sufficient to use the values in registers of the SUT for finding counterexamples). I would like to discuss this first and we should then write a short documentation that this will not check equivalence between two automata in general!

OK, this change can be separated and/or reverted However, note that the previous code was not using its sys2reg argument in any interesting way: it was only passing it to a (trivial) method that was ignoring it and returned false in all cases. The only thing that such code achieves is creating confusion for the reader...

I will refactor this in a separate PR

fhowar · 2023-11-29T14:22:07Z

src/main/java/de/learnlib/ralib/learning/CounterexampleAnalysis.java

This should stay in the codebase but become a configuration option or a strategy object. @mtf90: how do we best align this with LearnLib?

I can revert this change, but note that the call to linearBackWardsSearch was commented out, so the code of that method was/is unused. How exactly do we know that the code does what is supposed to do (or will continue to do so after changes) if this code is not tested?

Perhaps a middle ground is to also comment out the method's code until it's decided to use this method (or have it as a configuration option or strategy, as you suggest). I will do that.

Removed code was put back in a commented out block.

Depending on which functionality you can provide, you may either use the "old" LocalSuffixFinder or Malte's acex framework. See here for an example.

src/main/java/de/learnlib/ralib/oracles/mto/OptimizedSymbolicSuffixBuilder.java

kostis requested review from fhowar and FredrikTaquist October 1, 2023 10:00

kostis force-pushed the remove-unused branch 2 times, most recently from 8b9aa4d to 360c197 Compare October 5, 2023 20:28

kostis added 19 commits October 6, 2023 14:52

Remove some unused code

1123d68

Remove unused logger

1c7bae5

Remove unnecessary returns

71ed131

Remove or comment out unused code

df7d8e3

Remove or comment out unused code from tests

15c884a

Remove or comment out some unused code

04a1645

The `IOEquivalenceTest` file seems to be semi-thought and in draft status.

Mark unused arguments and parameters as unused

4e9964c

Remove unused code from tests

06d1eaa

Remove unused parameters

5ec863d

Also, fix a copy-paste error in a test (which now enables it).

Remove unnecessary variable

1185b36

Comment out unused code

d4ac88c

Remove unused parameter

a7c55f3

Remove unused code

2cc365f

Changes to PrefixFinder and simplification of RaLambda's implementation

274bdfb

Comment out unused fields

44b5cd3

Comment out unused code

f5201c3

Remove unused internal class and methods

158c436

Comment out two unused methods

e0f8a84

Comment out an unused constructor method

1ca0537

kostis force-pushed the remove-unused branch from 360c197 to 1ca0537 Compare October 6, 2023 13:26

fhowar reviewed Oct 9, 2023

View reviewed changes

fhowar requested changes Nov 29, 2023

View reviewed changes

Comment out instead of removing code of linearBackWardsSearch

7a1ac01

fhowar approved these changes Feb 7, 2024

View reviewed changes

fhowar merged commit bb4db3a into main Feb 7, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove or comment out unused code #47

Remove or comment out unused code #47

kostis commented Oct 1, 2023 •

edited

Loading

fhowar left a comment

kostis commented Oct 9, 2023

fhowar commented Oct 9, 2023

fhowar Oct 9, 2023

kostis Oct 10, 2023 •

edited

Loading

fhowar Nov 29, 2023

mtf90 Nov 29, 2023

fhowar left a comment

fhowar Oct 9, 2023

fhowar Nov 29, 2023

fhowar Nov 29, 2023

kostis Nov 29, 2023

fhowar Dec 1, 2023

fhowar Nov 29, 2023

kostis Nov 29, 2023

fhowar Dec 1, 2023

fhowar Nov 29, 2023

kostis Nov 29, 2023

kostis Nov 29, 2023

mtf90 Nov 29, 2023

Remove or comment out unused code #47

Remove or comment out unused code #47

Conversation

kostis commented Oct 1, 2023 • edited Loading

fhowar left a comment

Choose a reason for hiding this comment

kostis commented Oct 9, 2023

fhowar commented Oct 9, 2023

Choose a reason for hiding this comment

kostis Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhowar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostis commented Oct 1, 2023 •

edited

Loading

kostis Oct 10, 2023 •

edited

Loading