From: Anna Dehof 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 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 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 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 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