fixed several crucial bugs related to dd bisimulation, tests now passing

src/storm/storage/dd/Add.cpp

@@ -831,8 +831,8 @@ namespace storm {
         }
 
         template<DdType LibraryType, typename ValueType>
-        void Add<LibraryType, ValueType>::exportToDot(std::string const& filename) const {
-            internalAdd.exportToDot(filename, this->getDdManager().getDdVariableNames());
+        void Add<LibraryType, ValueType>::exportToDot(std::string const& filename, bool showVariablesIfPossible) const {
+            internalAdd.exportToDot(filename, this->getDdManager().getDdVariableNames(), showVariablesIfPossible);
         }
         
         template<DdType LibraryType, typename ValueType>

src/storm/storage/dd/Add.h

@@ -618,7 +618,7 @@ namespace storm {
              *
              * @param filename The name of the file to which the DD is to be exported.
              */
-            void exportToDot(std::string const& filename) const override;
+            void exportToDot(std::string const& filename, bool showVariablesIfPossible = true) const override;
             
             /*!
              * Retrieves an iterator that points to the first meta variable assignment with a non-zero function value.

src/storm/storage/dd/Bdd.cpp

@@ -380,8 +380,8 @@ namespace storm {
         }
         
         template<DdType LibraryType>
-        void Bdd<LibraryType>::exportToDot(std::string const& filename) const {
-            internalBdd.exportToDot(filename, this->getDdManager().getDdVariableNames());
+        void Bdd<LibraryType>::exportToDot(std::string const& filename, bool showVariablesIfPossible) const {
+            internalBdd.exportToDot(filename, this->getDdManager().getDdVariableNames(), showVariablesIfPossible);
         }
         
         template<DdType LibraryType>

src/storm/storage/dd/Bdd.h

@@ -337,7 +337,7 @@ namespace storm {
             
             virtual uint_fast64_t getLevel() const override;
             
-            virtual void exportToDot(std::string const& filename) const override;
+            virtual void exportToDot(std::string const& filename, bool showVariablesIfPossible = true) const override;
             
             /*!
              * Retrieves the cube of all given meta variables.

src/storm/storage/dd/Dd.h

@@ -102,7 +102,7 @@ namespace storm {
              *
              * @param filename The name of the file to which the DD is to be exported.
              */
-            virtual void exportToDot(std::string const& filename) const = 0;
+            virtual void exportToDot(std::string const& filename, bool showVariablesIfPossible = true) const = 0;
             
             /*!
              * Retrieves the manager that is responsible for this DD.

src/storm/storage/dd/bisimulation/MdpPartitionRefiner.cpp

@@ -50,6 +50,9 @@ namespace storm {
             
             template<storm::dd::DdType DdType, typename ValueType>
             bool MdpPartitionRefiner<DdType, ValueType>::refineWrtStateActionRewards(storm::dd::Add<DdType, ValueType> const& stateActionRewards) {
+                STORM_LOG_TRACE("Refining with respect to state-action rewards.");
+                stateActionRewards.exportToDot("stateactrew.dot", false);
+                this->choicePartition.asAdd().exportToDot("choicepart.dot");
                 Partition<DdType, ValueType> newChoicePartition = this->signatureRefiner.refine(this->choicePartition, Signature<DdType, ValueType>(stateActionRewards));
                 if (newChoicePartition == this->choicePartition) {
                     return false;

src/storm/storage/dd/bisimulation/Partition.cpp

@@ -100,7 +100,7 @@ namespace storm {
             
             template<storm::dd::DdType DdType, typename ValueType>
             Partition<DdType, ValueType> Partition<DdType, ValueType>::createTrivialChoicePartition(storm::models::symbolic::NondeterministicModel<DdType, ValueType> const& model, std::pair<storm::expressions::Variable, storm::expressions::Variable> const& blockVariables) {
-                storm::dd::Bdd<DdType> choicePartitionBdd = !model.getIllegalSuccessorMask() && model.getManager().getEncoding(blockVariables.first, 0, false);
+                storm::dd::Bdd<DdType> choicePartitionBdd = !model.getIllegalMask().renameVariables(model.getRowVariables(), model.getColumnVariables()) && model.getManager().getEncoding(blockVariables.first, 0, false);
                 
                 // Store the partition as an ADD only in the case of CUDD.
                 if (DdType == storm::dd::DdType::CUDD) {

src/storm/storage/dd/bisimulation/PartitionRefiner.cpp

@@ -81,7 +81,7 @@ namespace storm {
                     result |= refineWrtStateRewards(rewardModel.getStateRewardVector());
                 }
                 if (rewardModel.hasStateActionRewards()) {
-                    result |= refineWrtStateRewards(rewardModel.getStateActionRewardVector());
+                    result |= refineWrtStateActionRewards(rewardModel.getStateActionRewardVector());
                 }
                 return result;
             }

src/storm/storage/dd/bisimulation/SignatureRefiner.cpp

@@ -84,7 +84,6 @@ namespace storm {
                     // Clear the caches.
                     signatureCache.clear();
                     reuseBlocksCache.clear();
-                    std::cout << "clearing reuse, " << reuseBlocksCache.size() << std::endl;
                     nextFreeBlockIndex = oldPartition.getNextFreeBlockIndex();
 
                     // Perform the actual recursive refinement step.
@@ -115,7 +114,6 @@ namespace storm {
                     // If we arrived at the constant zero node, then this was an illegal state encoding (we require
                     // all states to be non-deadlock).
                     if (partitionNode == Cudd_ReadZero(ddman)) {
-                        std::cout << "returning zero " << partitionNode << std::endl;
                         return partitionNode;
                     }
                     
@@ -123,7 +121,6 @@ namespace storm {
                     auto nodePair = std::make_pair(signatureNode, partitionNode);
                     auto it = signatureCache.find(nodePair);
                     if (it != signatureCache.end()) {
-                        std::cout << "cache hit " << it->second << std::endl;
                         // If so, we return the corresponding result.
                         return it->second;
                     }
@@ -132,18 +129,13 @@ namespace storm {
                     if (Cudd_IsConstant(nonBlockVariablesNode)) {
                         // If this is the first time (in this traversal) that we encounter this signature, we check
                         // whether we can assign the old block number to it.
-                        std::cout << "reuse size: " << reuseBlocksCache.size() << std::endl;
                         auto& reuseEntry = reuseBlocksCache[partitionNode];
-                        std::cout << "reused? " << reuseEntry.isReused() << std::endl;
                         if (!reuseEntry.isReused()) {
                             reuseEntry.setReused();
-                            std::cout << "setting " << partitionNode << " to being reused" << std::endl;
                             signatureCache[nodePair] = partitionNode;
-                            std::cout << "reuse cache hit " << partitionNode << std::endl;
                             return partitionNode;
                         } else {
                             DdNode* result = encodeBlock(nextFreeBlockIndex++);
-                            std::cout << "block encoding " << result << std::endl;
                             signatureCache[nodePair] = result;
                             return result;
                         }
@@ -199,9 +191,6 @@ namespace storm {
                         DdNode* elseResult = refine(partitionElse, signatureElse, offset == 0 ? Cudd_T(nondeterminismVariablesNode) : nondeterminismVariablesNode, Cudd_T(nonBlockVariablesNode));
                         Cudd_Ref(elseResult);
 
-                        std::cout << "got then " << thenResult << std::endl;
-                        std::cout << "got else " << elseResult << std::endl;
-
                         DdNode* result;
                         if (thenResult == elseResult) {
                             Cudd_Deref(thenResult);
@@ -211,7 +200,6 @@ namespace storm {
                             // Get the node to connect the subresults.
                             bool complemented = Cudd_IsComplement(thenResult);
                             result = cuddUniqueInter(ddman, Cudd_NodeReadIndex(nonBlockVariablesNode) + offset, Cudd_Regular(thenResult), complemented ? Cudd_Not(elseResult) : elseResult);
-                            std::cout << "got " << result << " as a result node, idx " << (Cudd_NodeReadIndex(nonBlockVariablesNode) + offset) << " and children " << Cudd_NodeReadIndex(thenResult) << " [" << Cudd_IsConstant(thenResult) << "] (" << thenResult << ") and " << Cudd_NodeReadIndex(elseResult) << " [" << Cudd_IsConstant(elseResult) << "] (" << elseResult << ")" << std::endl;
                             if (complemented) {
                                 result = Cudd_Not(result);
                             }
@@ -222,7 +210,6 @@ namespace storm {
                         // Store the result in the cache.
                         signatureCache[nodePair] = result;
                         
-                        std::cout << "returning " << result << " with index " << Cudd_NodeReadIndex(result) << ", const? " << Cudd_IsConstant(result) << std::endl;
                         return result;
                     }
                 }
@@ -494,10 +481,7 @@ namespace storm {
             
             template<storm::dd::DdType DdType, typename ValueType>
             Partition<DdType, ValueType> SignatureRefiner<DdType, ValueType>::refine(Partition<DdType, ValueType> const& oldPartition, Signature<DdType, ValueType> const& signature) {
-                oldPartition.asAdd().getDdManager().debugCheck();
-                oldPartition.asAdd().exportToDot("lastpart.dot");
                 Partition<DdType, ValueType> result = internalRefiner->refine(oldPartition, signature);
-                oldPartition.asAdd().getDdManager().debugCheck();
                 return result;
             }
             

src/storm/storage/dd/cudd/InternalCuddAdd.cpp

@@ -322,7 +322,7 @@ namespace storm {
         }
         
         template<typename ValueType>
-        void InternalAdd<DdType::CUDD, ValueType>::exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings) const {
+        void InternalAdd<DdType::CUDD, ValueType>::exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings, bool showVariablesIfPossible) const {
             // Build the name input of the DD.
             std::vector<char*> ddNames;
             std::string ddName("f");
@@ -339,7 +339,11 @@ namespace storm {
             // Open the file, dump the DD and close it again.
             FILE* filePointer = fopen(filename.c_str() , "w");
             std::vector<cudd::ADD> cuddAddVector = { this->getCuddAdd() };
-            ddManager->getCuddManager().DumpDot(cuddAddVector, &ddVariableNames[0], &ddNames[0], filePointer);
+            if (showVariablesIfPossible) {
+                ddManager->getCuddManager().DumpDot(cuddAddVector, ddVariableNames.data(), &ddNames[0], filePointer);
+            } else {
+                ddManager->getCuddManager().DumpDot(cuddAddVector, nullptr, &ddNames[0], filePointer);
+            }
             fclose(filePointer);
             
             // Finally, delete the names.

src/storm/storage/dd/cudd/InternalCuddAdd.h

@@ -494,7 +494,7 @@ namespace storm {
              * @param filename The name of the file to which the DD is to be exported.
              * @param ddVariableNamesAsString The names of the DD variables to display in the dot file.
              */
-            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings) const;
+            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings, bool showVariablesIfPossible = true) const;
             
             /*!
              * Retrieves an iterator that points to the first meta variable assignment with a non-zero function value.

src/storm/storage/dd/cudd/InternalCuddBdd.cpp

@@ -179,7 +179,7 @@ namespace storm {
             return static_cast<uint_fast64_t>(ddManager->getCuddManager().ReadPerm(this->getIndex()));
         }
         
-        void InternalBdd<DdType::CUDD>::exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings) const {
+        void InternalBdd<DdType::CUDD>::exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings, bool showVariablesIfPossible) const {
             // Build the name input of the DD.
             std::vector<char*> ddNames;
             std::string ddName("f");
@@ -196,7 +196,11 @@ namespace storm {
             // Open the file, dump the DD and close it again.
             std::vector<cudd::BDD> cuddBddVector = { this->getCuddBdd() };
             FILE* filePointer = fopen(filename.c_str() , "w");
-            ddManager->getCuddManager().DumpDot(cuddBddVector, &ddVariableNames[0], &ddNames[0], filePointer);
+            if (showVariablesIfPossible) {
+                ddManager->getCuddManager().DumpDot(cuddBddVector, ddVariableNames.data(), &ddNames[0], filePointer);
+            } else {
+                ddManager->getCuddManager().DumpDot(cuddBddVector, nullptr, &ddNames[0], filePointer);
+            }
             fclose(filePointer);
             
             // Finally, delete the names.

src/storm/storage/dd/cudd/InternalCuddBdd.h

@@ -327,7 +327,7 @@ namespace storm {
              * @param filename The name of the file to which the BDD is to be exported.
              * @param ddVariableNamesAsStrings The names of the variables to display in the dot file.
              */
-            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings) const;
+            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings, bool showVariablesIfPossible = true) const;
                         
             /*!
              * Converts a BDD to an equivalent ADD.

src/storm/storage/dd/sylvan/InternalSylvanAdd.cpp

@@ -783,7 +783,7 @@ namespace storm {
         }
         
         template<typename ValueType>
-        void InternalAdd<DdType::Sylvan, ValueType>::exportToDot(std::string const& filename, std::vector<std::string> const&) const {
+        void InternalAdd<DdType::Sylvan, ValueType>::exportToDot(std::string const& filename, std::vector<std::string> const&, bool) const {
             // Open the file, dump the DD and close it again.
             FILE* filePointer = fopen(filename.c_str() , "w");
             this->sylvanMtbdd.PrintDot(filePointer);

src/storm/storage/dd/sylvan/InternalSylvanAdd.h

@@ -497,7 +497,7 @@ namespace storm {
              * @param filename The name of the file to which the DD is to be exported.
              * @param ddVariableNamesAsString The names of the DD variables to display in the dot file.
              */
-            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings) const;
+            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings, bool showVariablesIfPossible = true) const;
             
             /*!
              * Retrieves an iterator that points to the first meta variable assignment with a non-zero function value.

src/storm/storage/dd/sylvan/InternalSylvanBdd.cpp

@@ -232,7 +232,7 @@ namespace storm {
             return this->getIndex();
         }
         
-        void InternalBdd<DdType::Sylvan>::exportToDot(std::string const& filename, std::vector<std::string> const&) const {
+        void InternalBdd<DdType::Sylvan>::exportToDot(std::string const& filename, std::vector<std::string> const&, bool) const {
             FILE* filePointer = fopen(filename.c_str() , "w");
             this->sylvanBdd.PrintDot(filePointer);
             fclose(filePointer);

src/storm/storage/dd/sylvan/InternalSylvanBdd.h

@@ -316,7 +316,7 @@ namespace storm {
              * @param filename The name of the file to which the BDD is to be exported.
              * @param ddVariableNamesAsStrings The names of the variables to display in the dot file.
              */
-            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings) const;
+            void exportToDot(std::string const& filename, std::vector<std::string> const& ddVariableNamesAsStrings, bool showVariablesIfPossible = true) const;
             
             /*!
              * Converts a BDD to an equivalent ADD.

src/test/storm/storage/SymbolicBisimulationDecompositionTest.cpp

@@ -116,7 +116,10 @@ TEST(SymbolicModelBisimulationDecomposition, AsynchronousLeader_Cudd) {
     // Preprocess model to substitute all constants.
     smd = smd.preprocess();
 
-    std::shared_ptr<storm::models::symbolic::Model<storm::dd::DdType::CUDD, double>> model = storm::builder::DdPrismModelBuilder<storm::dd::DdType::CUDD, double>().build(smd.asPrismProgram());
+    storm::parser::FormulaParser formulaParser;
+    std::shared_ptr<storm::logic::Formula const> formula = formulaParser.parseSingleFormulaFromString("Rmax=? [F \"elected\"]");
+    
+    std::shared_ptr<storm::models::symbolic::Model<storm::dd::DdType::CUDD, double>> model = storm::builder::DdPrismModelBuilder<storm::dd::DdType::CUDD, double>().build(smd.asPrismProgram(), *formula);
     
     storm::dd::BisimulationDecomposition<storm::dd::DdType::CUDD, double> decomposition(*model, storm::storage::BisimulationType::Strong);
     decomposition.compute();
@@ -128,9 +131,6 @@ TEST(SymbolicModelBisimulationDecomposition, AsynchronousLeader_Cudd) {
     EXPECT_TRUE(quotient->isSymbolicModel());
     EXPECT_EQ(500ul, (quotient->as<storm::models::symbolic::Mdp<storm::dd::DdType::CUDD, double>>()->getNumberOfChoices()));
     
-    storm::parser::FormulaParser formulaParser;
-    std::shared_ptr<storm::logic::Formula const> formula = formulaParser.parseSingleFormulaFromString("Rmax=? [F \"elected\"]");
-    
     std::vector<std::shared_ptr<storm::logic::Formula const>> formulas;
     formulas.push_back(formula);
     
@@ -138,9 +138,9 @@ TEST(SymbolicModelBisimulationDecomposition, AsynchronousLeader_Cudd) {
     decomposition2.compute();
     quotient = decomposition2.getQuotient();
     
-    EXPECT_EQ(19ul, quotient->getNumberOfStates());
-    EXPECT_EQ(58ul, quotient->getNumberOfTransitions());
+    EXPECT_EQ(252ul, quotient->getNumberOfStates());
+    EXPECT_EQ(624ul, quotient->getNumberOfTransitions());
     EXPECT_EQ(storm::models::ModelType::Mdp, quotient->getType());
     EXPECT_TRUE(quotient->isSymbolicModel());
-    EXPECT_EQ(34ul, (quotient->as<storm::models::symbolic::Mdp<storm::dd::DdType::CUDD, double>>()->getNumberOfChoices()));
+    EXPECT_EQ(500ul, (quotient->as<storm::models::symbolic::Mdp<storm::dd::DdType::CUDD, double>>()->getNumberOfChoices()));
 }