path: root/sci-chemistry/ball/files/ball-1.4.1-Fixed-a-bug-in-the-AssignBondOrderProcessor.patch

From: Anna Dehof <anne@bioinf.uni-sb.de>
Date: Fri, 27 Apr 2012 18:41:52 +0200
Subject: Fixed a bug in the AssignBondOrderProcessor

---
 .../STRUCTURE/BONDORDERS/bondOrderAssignment.h     |    2 +-
 source/STRUCTURE/BONDORDERS/FPTBondOrderStrategy.C |   13 +++--
 source/STRUCTURE/assignBondOrderProcessor.C        |   27 +++++++----
 source/TEST/AssignBondOrderProcessor_test.C        |   50 +++++++++++++++-----
 4 files changed, 67 insertions(+), 25 deletions(-)

diff --git a/include/BALL/STRUCTURE/BONDORDERS/bondOrderAssignment.h b/include/BALL/STRUCTURE/BONDORDERS/bondOrderAssignment.h
index 57adc41..f8aab75 100644
--- a/include/BALL/STRUCTURE/BONDORDERS/bondOrderAssignment.h
+++ b/include/BALL/STRUCTURE/BONDORDERS/bondOrderAssignment.h
@@ -68,7 +68,7 @@ namespace BALL
 			float total_charge;
 			int node_expansions;
 			int queue_size;
-			
+
 			AtomContainer* ac;
 	};
 }
diff --git a/source/STRUCTURE/BONDORDERS/FPTBondOrderStrategy.C b/source/STRUCTURE/BONDORDERS/FPTBondOrderStrategy.C
index 107823e..6572d96 100644
--- a/source/STRUCTURE/BONDORDERS/FPTBondOrderStrategy.C
+++ b/source/STRUCTURE/BONDORDERS/FPTBondOrderStrategy.C
@@ -108,7 +108,8 @@ namespace BALL
 		}
 		else
 		{
-		/*	Log.info() << "AssignBondOrderProcessor: strategy FPT does not support computeNextSolution(). " << endl
+			// since we return a pointer, nothing to do here
+			/*	Log.info() << "AssignBondOrderProcessor: strategy FPT does not support computeNextSolution(). " << endl
 								 << "Please use the options Option::MAX_NUMBER_OF_SOLUTIONS or Option::COMPUTE_ALSO_NON_OPTIMAL_SOLUTIONS " << endl
 								 << "to compute additional solutions." << endl; */
 		}
@@ -1429,12 +1430,12 @@ namespace BALL
 
 	bool FPTBondOrderStrategy::DPBackTracking_::hasMoreSolutions() const
 	{
-		return (!queue_.empty() && (!max_num_solutions_ || (num_computed_solutions_ <= max_num_solutions_)));
+		return (!queue_.empty() && (!max_num_solutions_ || (num_computed_solutions_ < max_num_solutions_)));
 	}
 
 	void FPTBondOrderStrategy::DPBackTracking_::nextSolution()
 	{
-		if (queue_.empty() || max_heap_size_ == 0 || ((max_num_solutions_ > 0) && (num_computed_solutions_ > max_num_solutions_)))
+		if (queue_.empty() || (max_heap_size_ == 0) || ((max_num_solutions_ > 0) && (num_computed_solutions_ > max_num_solutions_)))
 		{
 			throw Exception::OutOfRange(__FILE__, __LINE__);
 		}
@@ -1987,7 +1988,9 @@ namespace BALL
 
 	bool FPTBondOrderStrategy::DPBackTrackingCombiner_::hasMoreSolutions() const
 	{
-		return (!priority_queue_.empty() || (getNextMinimumBackTracker_().second) < upper_bound_);
+		std::pair<Size, Penalty> next_min = getNextMinimumBackTracker_();
+
+		return (backtrackers_[next_min.first]->hasMoreSolutions() && (!priority_queue_.empty() || (next_min.second) < upper_bound_));
 	}
 
 	void FPTBondOrderStrategy::DPBackTrackingCombiner_::nextSolution()
@@ -2014,7 +2017,7 @@ namespace BALL
 		// compute next minimal Solution
 		std::pair<Size, Penalty> next_min = getNextMinimumBackTracker_();
 
-		if (next_min.second < upper_bound_ && (priority_queue_.empty() || next_min.second < priority_queue_.top().penalty))
+		if ((next_min.second < upper_bound_) && (priority_queue_.empty() || (next_min.second < priority_queue_.top().penalty)))
 		{
 			DPBackTracking_& min_back_tracker = *backtrackers_[next_min.first];
 
diff --git a/source/STRUCTURE/assignBondOrderProcessor.C b/source/STRUCTURE/assignBondOrderProcessor.C
index 9863a54..9443fb8 100644
--- a/source/STRUCTURE/assignBondOrderProcessor.C
+++ b/source/STRUCTURE/assignBondOrderProcessor.C
@@ -295,8 +295,20 @@ namespace BALL
 			 )
 		{
 			Log.error() << __FILE__ << " " << __LINE__
-				          << " : Error in options! FPT cannot be used with these options." << endl
-									<< " Consider switch to solution strategy ASTAR by setting Option::ALGORITHM to Algorithm::ASTAR." << endl
+				          << " : Error in options! FPT cannot be used with these option(s): ";
+			if (options.getReal(Option::BOND_LENGTH_WEIGHTING) > 0.)
+				Log.error() << "BOND_LENGTH_WEIGHTING ";
+			if (options.getBool(Option::ADD_HYDROGENS))
+				Log.error() << "ADD_HYDROGENS ";
+			if (options.getBool(Option::COMPUTE_ALSO_CONNECTIVITY))
+				Log.error() << "COMPUTE_ALSO_CONNECTIVITY ";
+			if (options.getBool(Option::OVERWRITE_SELECTED_BONDS))
+				Log.error() << "OVERWRITE_SELECTED_BONDS ";
+			if (!options.getBool(Option::OVERWRITE_SINGLE_BOND_ORDERS))
+				Log.error() << "OVERWRITE_SINGLE_BOND_ORDERS ";
+			Log.error() << endl;
+
+			Log.error() << " Consider switch to solution strategy ASTAR by setting Option::ALGORITHM to Algorithm::ASTAR." << endl
 									<< " Abort." << endl;
 			ret = false;
 		}
@@ -321,8 +333,8 @@ namespace BALL
 
 #ifdef DEBUG 
 cout << "  OPTIONS:" << endl;
-cout << " \t Algorithm: " <<  options[Option::Option::ALGORITHM] << endl;
-cout << " \t Heuristic: " <<  options[Option::Option::HEURISTIC] << endl;
+cout << " \t Algorithm: " <<  options[Option::ALGORITHM] << endl;
+cout << " \t Heuristic: " <<  options[Option::HEURISTIC] << endl;
 
 cout << " \t Overwrite bonds (single, double, triple, selected):"
 		 << options.getBool(Option::OVERWRITE_SINGLE_BOND_ORDERS) << " "
@@ -334,7 +346,7 @@ cout << " \t Overwrite bonds (single, double, triple, selected):"
 cout << " \t Add hydrogens : " << options.getBool(Option::ADD_HYDROGENS) << endl;
 cout << " \t Use fine penalty : " << options.getBool(Option::USE_FINE_PENALTY) << endl;
 cout << " \t Kekulizer: " << options.getBool(Option::KEKULIZE_RINGS)  << endl;
-cout << " \t Penalty file " << options[Option::Option::INIFile] << endl;
+cout << " \t Penalty file " << options[Option::INIFile] << endl;
 cout << " \t alpha: " << options[Option::BOND_LENGTH_WEIGHTING] << endl;
 cout << " \t max bond order: " << options[Option::MAX_BOND_ORDER] << endl;
 cout << " \t max number of solutions " << options[Option::MAX_NUMBER_OF_SOLUTIONS] << endl;
@@ -549,7 +561,7 @@ cout << "preassignPenaltyClasses_:" << preassignPenaltyClasses_() << " precomput
 					boost::shared_ptr<BondOrderAssignment> solution = strategy->computeNextSolution();
 
 					// Do we have a solution? 
-					if (!solution)
+					if (!solution || !solution->valid)
 					{
 						Log.info() << "AssignBondOrderProcessor: No valid bond order assignment found!" << endl;
 #if defined DEBUG_TIMER					
@@ -1237,7 +1249,6 @@ cout << " ~~~~~~~~ added hydrogen dump ~~~~~~~~~~~~~~~~" << endl;
 		                       AssignBondOrderProcessor::Default::APPLY_FIRST_SOLUTION);
 	}
 
-
 	bool AssignBondOrderProcessor::apply(Position i)
 	{
 		bool result = false;
@@ -1370,7 +1381,7 @@ cout << " ~~~~~~~~ added hydrogen dump ~~~~~~~~~~~~~~~~" << endl;
 
 		boost::shared_ptr<BondOrderAssignment> solution = strategy->computeNextSolution();
 
-		if (solution)
+		if (solution && solution->valid)
 		{
 			solutions_.push_back(*solution);
 			found_a_sol = true;
diff --git a/source/TEST/AssignBondOrderProcessor_test.C b/source/TEST/AssignBondOrderProcessor_test.C
index 6dfafcc..fdf92d1 100644
--- a/source/TEST/AssignBondOrderProcessor_test.C
+++ b/source/TEST/AssignBondOrderProcessor_test.C
@@ -201,6 +201,8 @@ RESULT
 
 
 CHECK(check Options for consistency)
+	Log.error().disableOutput();
+
 	AssignBondOrderProcessor testbop;
 	testbop.setDefaultOptions();
 	TEST_EQUAL(testbop.hasValidOptions(), true)
@@ -208,21 +210,17 @@ CHECK(check Options for consistency)
 	testbop.setDefaultOptions();
 	testbop.options.set(AssignBondOrderProcessor::Option::ALGORITHM, AssignBondOrderProcessor::Algorithm::FPT);
 	testbop.options.set(AssignBondOrderProcessor::Option::ADD_HYDROGENS, true);
-
 	TEST_EQUAL(testbop.hasValidOptions(), false)
 
 	testbop.setDefaultOptions();
 	testbop.options.set(AssignBondOrderProcessor::Option::ALGORITHM, AssignBondOrderProcessor::Algorithm::FPT);
 	testbop.options.set(AssignBondOrderProcessor::Option::OVERWRITE_SELECTED_BONDS, true);
-
 	TEST_EQUAL(testbop.hasValidOptions(), false)
 
 	testbop.setDefaultOptions();
 	testbop.options.set(AssignBondOrderProcessor::Option::ALGORITHM, AssignBondOrderProcessor::Algorithm::FPT);
 	testbop.options.set(AssignBondOrderProcessor::Option::MAX_NUMBER_OF_SOLUTIONS, 0);
-
-	TEST_EQUAL(testbop.hasValidOptions(), false)
-
+	TEST_EQUAL(testbop.hasValidOptions(), true)
 
 	testbop.setDefaultOptions();
 	testbop.options.set(AssignBondOrderProcessor::Option::ALGORITHM, AssignBondOrderProcessor::Algorithm::FPT);
@@ -230,9 +228,8 @@ CHECK(check Options for consistency)
 	testbop.options.set(AssignBondOrderProcessor::Option::OVERWRITE_SINGLE_BOND_ORDERS, true);
 	testbop.options.set(AssignBondOrderProcessor::Option::COMPUTE_ALSO_CONNECTIVITY, true);
 	testbop.options.set(AssignBondOrderProcessor::Option::MAX_NUMBER_OF_SOLUTIONS, 0);
-
 	TEST_EQUAL(testbop.hasValidOptions(), false)
-
+	Log.error().enableOutput();
 RESULT
 
 ///////////////////////  ALGORITHMS     //////////////////////
@@ -841,10 +838,11 @@ CHECK(getTotalPenalty(Position i) and operator() FPT single solution)
 	System sys2;
 	MOL2File mol_in2(BALL_TEST_DATA_PATH(AssignBondOrderProcessor_test_AN06.mol2), std::ios::in);
 	mol_in2 >> sys2;
+	TEST_EQUAL(testbop.getNumberOfComputedSolutions(), 10)
+
 	sys2.apply(testbop);
 	TEST_REAL_EQUAL(testbop.getTotalPenalty(0), 2.f)//0.00625) 
 
-
 	System sys3;
 	MOL2File mol_in3(BALL_TEST_DATA_PATH(AssignBondOrderProcessor_test_BEWCUB.mol2), std::ios::in);
 	mol_in3 >> sys3;
@@ -856,6 +854,7 @@ CHECK(getTotalPenalty(Position i) and operator() FPT single solution)
 	MOL2File mol15(BALL_TEST_DATA_PATH(AssignBondOrderProcessor_test_CUDJAM_sol_5.mol2), std::ios::in);
 	mol15 >> sys15;
 	sys15.apply(testbop);
+
 	TEST_REAL_EQUAL(testbop.getTotalPenalty(1), 2.f ) //0.0015528 )
 	TEST_REAL_EQUAL(testbop.getTotalPenalty(2), 5.f ) //0.00388199)
 	TEST_REAL_EQUAL(testbop.getTotalPenalty(3), 7.f ) //0.00543478)
@@ -904,6 +903,34 @@ CHECK(getTotalPenalty(Position i) and operator() FPT single solution)
 
 RESULT
 
+CHECK(operator() FPT vs A*)
+	AssignBondOrderProcessor testbop_a;
+	testbop_a.options.set(AssignBondOrderProcessor::Option::ALGORITHM,AssignBondOrderProcessor::Algorithm::A_STAR);
+	testbop_a.options.setBool(AssignBondOrderProcessor::Option::COMPUTE_ALSO_NON_OPTIMAL_SOLUTIONS, true);
+
+	AssignBondOrderProcessor testbop_fpt;
+	testbop_fpt.options.set(AssignBondOrderProcessor::Option::ALGORITHM,AssignBondOrderProcessor::Algorithm::FPT);
+	testbop_fpt.options.setBool(AssignBondOrderProcessor::Option::COMPUTE_ALSO_NON_OPTIMAL_SOLUTIONS, true);
+
+	System sys4;
+	MOL2File mol4(BALL_TEST_DATA_PATH(AssignBondOrderProcessor_test_CITSED10_sol_6.mol2), std::ios::in);
+	mol4 >> sys4;
+	sys4.apply(testbop_a);
+	sys4.apply(testbop_fpt);
+	TEST_REAL_EQUAL(testbop_a.getNumberOfComputedSolutions(), testbop_fpt.getNumberOfComputedSolutions())
+
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(0), testbop_fpt.getTotalPenalty(0)) //1.f )
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(1), testbop_fpt.getTotalPenalty(1)) //1.f )
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(2), testbop_fpt.getTotalPenalty(2)) //32.f)
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(3), testbop_fpt.getTotalPenalty(3)) //34.f)
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(4), testbop_fpt.getTotalPenalty(4)) //34.f)
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(5), testbop_fpt.getTotalPenalty(5)) //34.f)
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(6), testbop_fpt.getTotalPenalty(6)) //66.f)
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(7), testbop_fpt.getTotalPenalty(7)) //66.f)
+	TEST_REAL_EQUAL(testbop_a.getTotalPenalty(8), testbop_fpt.getTotalPenalty(8)) //67.f)
+
+RESULT
+
 
 CHECK(getTotalCharge(Position i))
 	// This feature is experimental!! 
@@ -1023,7 +1050,7 @@ CHECK(computeNextSolution() using ILP)
 	MOL2File mol_in(BALL_TEST_DATA_PATH(AssignBondOrderProcessor_test_AN06.mol2), std::ios::in);
 	mol_in >> sys;
 	sys.apply(testbop);
-	TEST_EQUAL(testbop.getNumberOfComputedSolutions(),1)
+	TEST_EQUAL(testbop.getNumberOfComputedSolutions(), 1)
 	TEST_REAL_EQUAL(testbop.getTotalPenalty(0), 2.f)//0.00625)// 2.)
 	TEST_EQUAL(testbop.computeNextSolution(), true)
 	TEST_REAL_EQUAL(testbop.getTotalPenalty(1), 32.f)//0.1)//32.)
@@ -1091,9 +1118,10 @@ CHECK(computeNextSolution() using FPT)
 	MOL2File mol_in(BALL_TEST_DATA_PATH(AssignBondOrderProcessor_test_AN06.mol2), std::ios::in);
 	mol_in >> sys;
 	sys.apply(testbop);
-	TEST_EQUAL(testbop.getNumberOfComputedSolutions(),1)
+	TEST_EQUAL(testbop.getNumberOfComputedSolutions(), 1)
 	TEST_REAL_EQUAL(testbop.getTotalPenalty(0), 2.f)//0.00625)// 2.)
-	TEST_EQUAL(testbop.computeNextSolution(), false)
+	bool test = testbop.computeNextSolution();
+	TEST_EQUAL(test, false)
 
 RESULT