Phase saving, restarts, and heuristic performance logs #202

AntoniusW · 2019-10-07T12:45:19Z

This is a preview PR adding two standard SAT/ASP techniques.
Restarts in conjunction with the restricted guessing on applicable rules (in order to compute computation sequences as in other lazy-grounding ASP systems), however, have a negative impact on search performance. Unless further advances for lazy-grounding search to remedy that performance impact are available, this PR shall not be merged.

- Assignment provides getLastValue (i.e. phase). - VSIDSWithPhaseSaving is VSIDS but uses saved phase from Assignment. - BranchingHeuristicFactory knows new VSIDSWithPhaseSaving heuristics. - HeapOfActiveAtoms polished, growForMaxAtomId added. - TrailAssignment saves the phase (last truth value) of variables.

- CommandLineParser and SystemConfig have option to enable restarts. - DefaultSolver runs restarts if enabled. - Add MixedRestartStrategy combining Luby and dynamic (EMA) restarts. - Enhanced performance logs for conflicts and restarts

- ChainedBranchingHeuristics gives access to first-run heuristics. - HeapOfActiveAtoms reports activity increment for normalization. - VSIDSWithPhaseSaving logs * the overall, normalized decrease in activity of the seelected atoms, * the number of most-active choices thrown away because they were not active choice points at the time. - PerformanceLog prints heuristics information if DefaultSolver is runnning with VSIDSWithPhaseSaving

with phase saving. - CommandLineParser and SystemConfig accept initial phase settings. - AtomChoiceRelation stores relation between ordinary atoms and the choice points that influence them. - NaiveGrounder and ProgramAnalyzingGrounder provide AtomChoiceRelation. - NoGoodGenerator fills AtomChoiceRelation. - BranchingHeuristicFactory sets AtomChoiceRelation for VSIDSWithPhaseSaving. - PhaseInitializerFactory provides different initial phase settings. - VSIDSWithPhaseSaving uses AtomChoiceRelation for activity increments. - SolverFactory sets chosen phase initializer. - TrailAssignment considers initial phase value, if phase was not set. - Tests set a phase initializer if needed.

codecov · 2019-10-23T02:57:10Z

Codecov Report

Merging #202 (fcd2436) into master (21d074a) will increase coverage by 0.00%.
The diff coverage is 70.64%.

@@            Coverage Diff             @@
##             master     #202    +/-   ##
==========================================
  Coverage     69.51%   69.51%            
- Complexity     2092     2159    +67     
==========================================
  Files           182      188     +6     
  Lines          8006     8261   +255     
  Branches       1416     1443    +27     
==========================================
+ Hits           5565     5743   +178     
- Misses         2086     2152    +66     
- Partials        355      366    +11

Impacted Files	Coverage Δ
.../java/at/ac/tuwien/kr/alpha/commons/util/Util.java	`9.09% <0.00%> (-0.29%)`	⬇️
.../at/ac/tuwien/kr/alpha/core/common/Assignment.java	`86.66% <ø> (ø)`
.../tuwien/kr/alpha/core/grounder/ChoiceRecorder.java	`89.74% <0.00%> (+10.19%)`	⬆️
...ien/kr/alpha/core/solver/MixedRestartStrategy.java	`0.00% <0.00%> (ø)`
...olver/heuristics/AlphaHeadMustBeTrueHeuristic.java	`0.00% <0.00%> (ø)`
...ore/solver/heuristics/PhaseInitializerFactory.java	`23.52% <23.52%> (ø)`
.../ac/tuwien/kr/alpha/core/solver/DefaultSolver.java	`64.84% <40.00%> (-1.22%)`	⬇️
.../tuwien/kr/alpha/core/solver/heuristics/VSIDS.java	`91.48% <71.42%> (-0.18%)`	⬇️
.../tuwien/kr/alpha/app/config/CommandLineParser.java	`82.26% <76.19%> (-0.63%)`	⬇️
...a/core/solver/heuristics/VSIDSWithPhaseSaving.java	`76.92% <76.92%> (ø)`
... and 19 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 21d074a...fcd2436. Read the comment docs.

- In HeapOfActiveAtoms several members/methods package-private now to allow overriding. - Add HeapOfRelatedChoiceAtoms using AtomChoiceRelation to only record and initialize choice points. - Moved ChoiceManager update in DefaultSolver to correctly know which atoms are choice points. - Improved logging in VSIDSWithPhaseSaving and PerformanceLog.

- Move getAtomChoiceRelation() from NaiveGrounder to Grounder, implement method also in DummyGrounder and ChoiceGrounder. - HeapOfActiveAtoms keeps track of literals occurring in the heap. - More logging stats from VSIDSWithPhaseSaving. - ChoiceInfluenceManager: simplified callbackOnChange

# Conflicts: # src/main/java/at/ac/tuwien/kr/alpha/config/CommandLineParser.java # src/main/java/at/ac/tuwien/kr/alpha/config/SystemConfig.java # src/main/java/at/ac/tuwien/kr/alpha/grounder/NaiveGrounder.java # src/test/java/at/ac/tuwien/kr/alpha/grounder/NaiveGrounderTest.java # src/test/java/at/ac/tuwien/kr/alpha/solver/LearnedNoGoodDeletionTest.java

…e_master Merge master into phase_saving_and_restarts

# Conflicts: # src/main/java/at/ac/tuwien/kr/alpha/config/CommandLineParser.java

…e_master Merge master into phase_saving_and_restarts

rtaupe · 2020-04-24T07:27:35Z

Now #239 pops up also here. We should really fix this issue ...

src/main/java/at/ac/tuwien/kr/alpha/AnswerSetToXlsxWriter.java

rtaupe

Thank you for this contribution!
I have commented upon some issues that should, in my opinion, be addressed before merging.
So far I have only read the code, a second review concerned with performance will follow.
Please also address the Codecov report.

src/main/java/at/ac/tuwien/kr/alpha/config/SystemConfig.java

src/main/java/at/ac/tuwien/kr/alpha/grounder/ChoiceRecorder.java

rtaupe · 2020-04-24T07:38:58Z

src/main/java/at/ac/tuwien/kr/alpha/solver/heuristics/HeapOfRelatedChoiceAtoms.java

+ *
+ * Copyright (c) 2019, the Alpha Team.
+ */
+public class HeapOfRelatedChoiceAtoms extends HeapOfActiveAtoms {


Is this an improved version of, and thus a replacement for HeapOfActiveChoicePoints? If yes, this fact should be documented and HeapOfActiveChoicePoints should be deprecated or removed.

The main reason for this class to exists is the initialization of variables. In HeapOfActiveAtoms a new nogood leads to activities of literals occuring in the nogood to be updated, while here, not the literals themselves are updated, but the related choice atoms of those literals occurring in the nogood. This seems to be much more in line with the idea of having good values for choice atoms (and not the "useless" ordinary atoms).

Note that HeapOfActiveChoicePoints works in a different direction, namely by referring to related atoms when activities are incremented. For that class, atoms are related whenever they appear in the same nogood. The VSIDSWithPhaseSaving heuristic, however, uses the knowledge of atom relationships that it obtains from the grounder via the AtomChoiceRelation. (I currently do however not remember whether the obtained information in both cases is the same or different. The information flow in VSIDSWithPhaseSaving and HeapOfRelatedChoiceAtoms seems to be more direct.)

I think, it is worthwhile to consider merging both approaches. If I recall correctly, I did not do it, because there were some differences and I did not want to change all the other heuristics we have so far without any discussion.

src/main/java/at/ac/tuwien/kr/alpha/grounder/NoGoodGenerator.java

src/main/java/at/ac/tuwien/kr/alpha/grounder/ProgramAnalyzingGrounder.java

src/main/java/at/ac/tuwien/kr/alpha/solver/heuristics/HeapOfActiveAtoms.java

rtaupe · 2020-04-24T08:19:13Z

src/main/java/at/ac/tuwien/kr/alpha/solver/heuristics/HeapOfRelatedChoiceAtoms.java

+			for (Integer relatedChoiceAtom : atomChoiceRelation.getRelatedChoiceAtoms(atomOf(literal))) {
+				if (!incrementedActivityScores[relatedChoiceAtom]) { // update initial value as long as not incremented yet by VSIDS
+					double score = moms.getScore(relatedChoiceAtom);
+					if (score > 0.0) {


Please re-use the existing code in at.ac.tuwien.kr.alpha.solver.heuristics.HeapOfActiveAtoms#initActivityMOMs instead of just copying it.

rtaupe · 2020-04-24T08:21:53Z

src/main/java/at/ac/tuwien/kr/alpha/solver/heuristics/VSIDSWithPhaseSaving.java

@@ -0,0 +1,226 @@
+/**
+ * Copyright (c) 2018-2019 Siemens AG


In my opinion, this implementation is independent from VSIDS enough for the copyright attribution to Siemens not to be necessary.

src/main/java/at/ac/tuwien/kr/alpha/solver/heuristics/VSIDSWithPhaseSaving.java

- Extend Util.join with toString method parameter.

- Change initial phase values from strings to enum. - Add PhaseInitializer for RULESTRUEATOMSFALSE.

rtaupe

Some more change requests.
One of them (the NullPointerException) is critical!

rtaupe · 2020-05-04T08:52:17Z

src/main/java/at/ac/tuwien/kr/alpha/config/CommandLineParser.java

+		.desc("enable the usage of (dynamic and static) restarts (default: "
+			+ SystemConfig.DEFAULT_ENABLE_RESTARTS + ")")
+		.build();
+	private static final Option OPT_INITIAL_PHASE = Option.builder("ph").longOpt("initialPhase").hasArg(true).argName("initializer")


Since this option is only meaningful if the VSIDS_PHASE_SAVING heuristic is used, a warning should be issued if it is used in conjunction with any other heuristic.

This is a good point and touches on a larger issue: what we would need is to check whether the whole set of given options actually makes sense. For the time being, I'll change the description so that it becomes clear that this only works for the VSIDS_WITH_PHASE_SAVING heuristic.

Added to issue #302 as a condition to check eventually.

src/main/java/at/ac/tuwien/kr/alpha/config/SystemConfig.java

src/main/java/at/ac/tuwien/kr/alpha/grounder/structure/AtomChoiceRelation.java

…default_initial_phase Use enum instead of string for default initial phase

rtaupe · 2020-05-26T10:49:24Z

There is a bug: #255
(I don´t know if it is related to this pull request, but it occurs on this branch.)

# Conflicts: # src/main/java/at/ac/tuwien/kr/alpha/AnswerSetToXlsxWriter.java # src/main/java/at/ac/tuwien/kr/alpha/Main.java # src/main/java/at/ac/tuwien/kr/alpha/api/mapper/AnswerSetToWorkbookMapper.java # src/main/java/at/ac/tuwien/kr/alpha/config/CommandLineParser.java # src/main/java/at/ac/tuwien/kr/alpha/config/InputConfig.java # src/main/java/at/ac/tuwien/kr/alpha/config/SystemConfig.java # src/main/java/at/ac/tuwien/kr/alpha/grounder/NaiveGrounder.java # src/main/java/at/ac/tuwien/kr/alpha/grounder/NoGoodGenerator.java # src/main/java/at/ac/tuwien/kr/alpha/grounder/RuleGroundingOrders.java # src/main/java/at/ac/tuwien/kr/alpha/grounder/transformation/CardinalityNormalization.java # src/test/java/at/ac/tuwien/kr/alpha/AnswerSetToXlsxWriterTest.java # src/test/java/at/ac/tuwien/kr/alpha/api/AlphaTest.java # src/test/java/at/ac/tuwien/kr/alpha/grounder/DummyGrounder.java # src/test/java/at/ac/tuwien/kr/alpha/grounder/NaiveGrounderTest.java # src/test/java/at/ac/tuwien/kr/alpha/grounder/RuleGroundingOrderTest.java # src/test/java/at/ac/tuwien/kr/alpha/solver/LearnedNoGoodDeletionTest.java

…to phase_saving_and_restarts

- Introduced AbstractVSIDS, bundling code common to all VSIDS implementations. - VSIDS refactored and extends AbstractVSIDS now. - VSIDSWithPhaseSaving refactored and extends AbstractVSIDS now.

# Conflicts: # alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/config/SystemConfig.java # alpha-cli-app/src/main/java/at/ac/tuwien/kr/alpha/app/config/CommandLineParser.java # alpha-cli-app/src/test/java/at/ac/tuwien/kr/alpha/app/config/CommandLineParserTest.java # alpha-cli-app/src/test/java/at/ac/tuwien/kr/alpha/app/mappers/AnswerSetToWorkbookMapperTest.java # alpha-commons/src/main/java/at/ac/tuwien/kr/alpha/commons/literals/ComparisonLiteralImpl.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/common/NoGood.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/grounder/ChoiceRecorder.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/grounder/Grounder.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/grounder/NaiveGrounder.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/grounder/NoGoodGenerator.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/programs/structure/AtomChoiceRelation.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/DefaultSolver.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/MixedRestartStrategy.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/NaiveNoGoodStore.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/NoGoodCounter.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/PerformanceLog.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/SolverFactory.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/TrailAssignment.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/AbstractVSIDS.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/BranchingHeuristicFactory.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/ChainedBranchingHeuristics.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/HeapOfActiveAtoms.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/HeapOfRelatedChoiceAtoms.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/PhaseInitializerFactory.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/VSIDS.java # alpha-core/src/main/java/at/ac/tuwien/kr/alpha/core/solver/heuristics/VSIDSWithPhaseSaving.java # alpha-core/src/test/java/at/ac/tuwien/kr/alpha/core/grounder/ChoiceGrounder.java # alpha-core/src/test/java/at/ac/tuwien/kr/alpha/core/grounder/DummyGrounder.java

- Extract InitialAtomPhase from PhaseInitializerFactory. - Fix missing/wrong imports.

AntoniusW added 3 commits September 30, 2019 00:50

Adding restarts (default disabled).

2904653

- CommandLineParser and SystemConfig have option to enable restarts. - DefaultSolver runs restarts if enabled. - Add MixedRestartStrategy combining Luby and dynamic (EMA) restarts. - Enhanced performance logs for conflicts and restarts

AntoniusW added the blocked label Oct 7, 2019

AntoniusW added 4 commits November 7, 2019 03:32

Fix bug in AlphaHeadMustBeTrueHeuristic.

6143e82

Fix Luby restart sequence computation.

cd6ea23

rtaupe mentioned this pull request Nov 12, 2019

WIP: Grounding strategies and phase saving and restarts #206

Closed

rtaupe and others added 5 commits February 12, 2020 09:25

Fix merge ab122e3

a6ccf70

Merge pull request #222 from alpha-asp/phase_saving_and_restarts_merg…

a7e66ce

…e_master Merge master into phase_saving_and_restarts

Merge branch 'master' into phase_saving_and_restarts_merge_master

0fbb0e9

# Conflicts: # src/main/java/at/ac/tuwien/kr/alpha/config/CommandLineParser.java

Merge pull request #224 from alpha-asp/phase_saving_and_restarts_merg…

f922794

…e_master Merge master into phase_saving_and_restarts

AntoniusW removed the blocked label Apr 15, 2020

AntoniusW requested a review from rtaupe April 15, 2020 14:14

Merge branch 'master' into phase_saving_and_restarts

689dd00

rtaupe reviewed Apr 24, 2020

View reviewed changes

src/main/java/at/ac/tuwien/kr/alpha/AnswerSetToXlsxWriter.java Outdated Show resolved Hide resolved

rtaupe requested changes Apr 24, 2020

View reviewed changes

AntoniusW added 4 commits May 1, 2020 20:36

Better toString in ChoiceRecorder.

c944b7e

- Extend Util.join with toString method parameter.

Fix checkstyle.

3770706

Add initial phase setting rules true, rest false.

f30057a

- Change initial phase values from strings to enum. - Add PhaseInitializer for RULESTRUEATOMSFALSE.

Address review comments and use array in AtomChoiceRelation.

0650096

rtaupe requested changes May 4, 2020

View reviewed changes

AntoniusW added 3 commits May 4, 2020 12:52

Remove unnecessary parameter of TrailAssignment.

d0229b4

Fix NullPointerException in AtomChoiceRelation.

7d992c2

Make AtomChoiceRelation reflexive on choice atoms.

48051e6

AntoniusW and others added 9 commits May 6, 2020 03:54

Clean-up logic in HeapOfActiveAtoms.

0589715

Use default PhaseInitializer in TrailAssignment.

d9180c4

Small polish.

4b8566a

Use enum instead of string for default initial phase

e63651a

Test parsing of -ph CLI parameter

2e26f69

Small polish.

340ae36

Merge pull request #253 from alpha-asp/phase_saving_and_restarts_fix_…

fdca492

…default_initial_phase Use enum instead of string for default initial phase

Merge upstream branch phase_saving_and_restarts into local.

4bb2cf8

Merge branch 'master' into phase_saving_and_restarts

7855224

AntoniusW added 9 commits September 20, 2021 22:58

Improve cmdline description of initial_phase flag.

e968afa

Fix merge conflicts.

7257348

Merge branch 'phase_saving_and_restarts' of github:AntoniusW/Alpha in…

a0f87e9

…to phase_saving_and_restarts

Merge branch 'master' into phase_saving_and_restarts

2fcf5d7

Refactor VSIDS implementations, extract common fields and methods.

af8a944

- Introduced AbstractVSIDS, bundling code common to all VSIDS implementations. - VSIDS refactored and extends AbstractVSIDS now. - VSIDSWithPhaseSaving refactored and extends AbstractVSIDS now.

Fix copyright statement as per review request.

419b819

Modularize phase saving and fix merge.

fcd2436

- Extract InitialAtomPhase from PhaseInitializerFactory. - Fix missing/wrong imports.

AntoniusW mentioned this pull request Dec 7, 2021

Validate SystemConfig #302

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phase saving, restarts, and heuristic performance logs #202

Phase saving, restarts, and heuristic performance logs #202

AntoniusW commented Oct 7, 2019 •

edited

Loading

codecov bot commented Oct 23, 2019 •

edited

Loading

rtaupe commented Apr 24, 2020

rtaupe left a comment

rtaupe Apr 24, 2020

AntoniusW May 3, 2020

rtaupe Apr 24, 2020

rtaupe Apr 24, 2020

rtaupe left a comment

rtaupe May 4, 2020

AntoniusW Sep 20, 2021

AntoniusW Dec 7, 2021

rtaupe commented May 26, 2020

Phase saving, restarts, and heuristic performance logs #202

Are you sure you want to change the base?

Phase saving, restarts, and heuristic performance logs #202

Conversation

AntoniusW commented Oct 7, 2019 • edited Loading

codecov bot commented Oct 23, 2019 • edited Loading

Codecov Report

rtaupe commented Apr 24, 2020

rtaupe left a comment

Choose a reason for hiding this comment

rtaupe Apr 24, 2020

Choose a reason for hiding this comment

AntoniusW May 3, 2020

Choose a reason for hiding this comment

rtaupe Apr 24, 2020

Choose a reason for hiding this comment

rtaupe Apr 24, 2020

Choose a reason for hiding this comment

rtaupe left a comment

Choose a reason for hiding this comment

rtaupe May 4, 2020

Choose a reason for hiding this comment

AntoniusW Sep 20, 2021

Choose a reason for hiding this comment

AntoniusW Dec 7, 2021

Choose a reason for hiding this comment

rtaupe commented May 26, 2020

AntoniusW commented Oct 7, 2019 •

edited

Loading

codecov bot commented Oct 23, 2019 •

edited

Loading