From 5224d3d067e37329527b4514144a8cb95eb60042 Mon Sep 17 00:00:00 2001 From: Viktor Govako Date: Wed, 23 Aug 2023 14:36:19 -0300 Subject: [PATCH] [search] Increased PreRankerResult batch size to 1000. Signed-off-by: Viktor Govako --- search/intermediate_result.cpp | 12 ++---- search/pre_ranker.cpp | 41 ++++++++++++++----- search/pre_ranker.hpp | 21 ++++++---- search/ranker.cpp | 13 ++++-- .../pre_ranker_test.cpp | 14 ++----- .../search_quality_tests/real_mwm_tests.cpp | 34 +++++++++++++-- 6 files changed, 91 insertions(+), 44 deletions(-) diff --git a/search/intermediate_result.cpp b/search/intermediate_result.cpp index 91f33adbf6c47..72d535c561c30 100644 --- a/search/intermediate_result.cpp +++ b/search/intermediate_result.cpp @@ -81,15 +81,15 @@ bool PreRankerResult::LessRankAndPopularity(PreRankerResult const & lhs, PreRank return lhs.m_info.m_rank > rhs.m_info.m_rank; if (lhs.m_info.m_popularity != rhs.m_info.m_popularity) return lhs.m_info.m_popularity > rhs.m_info.m_popularity; + + /// @todo Remove this epilog when we will have _enough_ ranks and popularities in data. return lhs.m_info.m_distanceToPivot < rhs.m_info.m_distanceToPivot; } // static bool PreRankerResult::LessDistance(PreRankerResult const & lhs, PreRankerResult const & rhs) { - if (lhs.m_info.m_distanceToPivot != rhs.m_info.m_distanceToPivot) - return lhs.m_info.m_distanceToPivot < rhs.m_info.m_distanceToPivot; - return lhs.m_info.m_rank > rhs.m_info.m_rank; + return lhs.m_info.m_distanceToPivot < rhs.m_info.m_distanceToPivot; } // static @@ -121,11 +121,7 @@ bool PreRankerResult::LessByExactMatch(PreRankerResult const & lhs, PreRankerRes if (lScore != rScore) return lScore; - auto const byTokens = CompareByTokensMatch(lhs, rhs); - if (byTokens != 0) - return byTokens == -1; - - return LessDistance(lhs, rhs); + return CompareByTokensMatch(lhs, rhs) == -1; } bool PreRankerResult::CategoriesComparator::operator()(PreRankerResult const & lhs, diff --git a/search/pre_ranker.cpp b/search/pre_ranker.cpp index 3eb1d4c1c2d55..00dc30ae73896 100644 --- a/search/pre_ranker.cpp +++ b/search/pre_ranker.cpp @@ -17,6 +17,7 @@ #include #include +#include namespace search { @@ -55,7 +56,8 @@ void SweepNearbyResults(m2::PointD const & eps, unordered_set const & } // namespace PreRanker::PreRanker(DataSource const & dataSource, Ranker & ranker) - : m_dataSource(dataSource), m_ranker(ranker), m_pivotFeatures(dataSource) +: m_dataSource(dataSource), m_ranker(ranker), m_pivotFeatures(dataSource) +, m_rndSeed(std::random_device()()) { } @@ -157,7 +159,14 @@ template class CompareIndices }; } // namespace -void PreRanker::Filter(bool viewportSearch) +void PreRanker::DbgFindAndLog(std::set const & ids) const +{ + for (auto const & r : m_results) + if (ids.count(r.GetId().m_index) > 0) + LOG(LDEBUG, (r)); +} + +void PreRanker::Filter() { auto const lessForUnique = [](PreRankerResult const & lhs, PreRankerResult const & rhs) { @@ -169,30 +178,40 @@ void PreRanker::Filter(bool viewportSearch) return PreRankerResult::CompareByTokensMatch(lhs, rhs) == -1; }; + /// @DebugNote + /// Use DbgFindAndLog to check needed ids before and after filtering + base::SortUnique(m_results, lessForUnique, base::EqualsBy(&PreRankerResult::GetId)); - if (viewportSearch) + if (m_params.m_viewportSearch) FilterForViewportSearch(); + // Viewport search ends here. if (m_results.size() <= BatchSize()) return; - vector indices(m_results.size()); - generate(indices.begin(), indices.end(), [n = 0] () mutable { return n++; }); - unordered_set filtered; + std::vector indices(m_results.size()); + std::generate(indices.begin(), indices.end(), [n = 0] () mutable { return n++; }); + std::unordered_set filtered; auto const iBeg = indices.begin(); auto const iMiddle = iBeg + BatchSize(); auto const iEnd = indices.end(); - nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessDistance, m_results)); + std::nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessDistance, m_results)); filtered.insert(iBeg, iMiddle); if (!m_params.m_categorialRequest) { - nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessRankAndPopularity, m_results)); + std::nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessRankAndPopularity, m_results)); filtered.insert(iBeg, iMiddle); - nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessByExactMatch, m_results)); + + // Shuffle to give a chance to far results, not only closest ones (see above). + // Search is not stable in rare cases, but we avoid increasing m_everywhereBatchSize. + /// @todo Move up, when we will have _enough_ ranks and popularities. + std::shuffle(iBeg, iEnd, std::mt19937(m_rndSeed)); + + std::nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessByExactMatch, m_results)); filtered.insert(iBeg, iMiddle); } else @@ -206,7 +225,7 @@ void PreRanker::Filter(bool viewportSearch) 2 * kPedestrianRadiusMeters; comparator.m_viewport = m_params.m_viewport; - nth_element(iBeg, iMiddle, iEnd, CompareIndices(comparator, m_results)); + std::nth_element(iBeg, iMiddle, iEnd, CompareIndices(comparator, m_results)); filtered.insert(iBeg, iMiddle); } @@ -221,7 +240,7 @@ void PreRanker::UpdateResults(bool lastUpdate) { FilterRelaxedResults(lastUpdate); FillMissingFieldsInPreResults(); - Filter(m_params.m_viewportSearch); + Filter(); m_numSentResults += m_results.size(); m_ranker.AddPreRankerResults(std::move(m_results)); m_results.clear(); diff --git a/search/pre_ranker.hpp b/search/pre_ranker.hpp index 305df7c62c399..ab26119bf7bb2 100644 --- a/search/pre_ranker.hpp +++ b/search/pre_ranker.hpp @@ -44,9 +44,11 @@ class PreRanker int m_scale = 0; - // Batch size for Everywhere search mode. For viewport search we limit search results number - // with SweepNearbyResults. - size_t m_everywhereBatchSize = 100; + // Batch size for Everywhere search mode. + // For viewport search we limit search results number with SweepNearbyResults. + // Increased to 1K, no problem to read 1-2K Features per search now, but the quality is much better. + /// @see BA_SanMartin test. + size_t m_everywhereBatchSize = 1000; // The maximum total number of results to be emitted in all batches. size_t m_limit = 0; @@ -76,11 +78,6 @@ class PreRanker m_haveFullyMatchedResult = true; } - // Computes missing fields for all pre-results. - void FillMissingFieldsInPreResults(); - - void Filter(bool viewportSearch); - // Emit a new batch of results up the pipeline (i.e. to ranker). // Use |lastUpdate| to indicate that no more results will be added. void UpdateResults(bool lastUpdate); @@ -137,8 +134,12 @@ class PreRanker void ClearCaches(); private: - void FilterForViewportSearch(); + // Computes missing fields for all pre-results. + void FillMissingFieldsInPreResults(); + void DbgFindAndLog(std::set const & ids) const; + void FilterForViewportSearch(); + void Filter(); void FilterRelaxedResults(bool lastUpdate); DataSource const & m_dataSource; @@ -166,6 +167,8 @@ class PreRanker std::unordered_set m_prevEmit; /// @} + unsigned m_rndSeed; + DISALLOW_COPY_AND_MOVE(PreRanker); }; } // namespace search diff --git a/search/ranker.cpp b/search/ranker.cpp index 7a1c24faa2dbb..5bd854bb1dc44 100644 --- a/search/ranker.cpp +++ b/search/ranker.cpp @@ -748,9 +748,14 @@ void Ranker::UpdateResults(bool lastUpdate) } else { - /// @note Here is _reverse_ order sorting, because bigger is better. - sort(m_tentativeResults.rbegin(), m_tentativeResults.rend(), - base::LessBy(&RankerResult::GetLinearModelRank)); + // Can get same Town features (from World) when searching in many MWMs. + base::SortUnique(m_tentativeResults, + [](RankerResult const & r1, RankerResult const & r2) + { + // Expect that linear rank is equal for the same features. + return r1.GetLinearModelRank() > r2.GetLinearModelRank(); + }, + base::EqualsBy(&RankerResult::GetID)); ProcessSuggestions(m_tentativeResults); } @@ -824,6 +829,8 @@ void Ranker::LoadCountriesTree() { m_regionInfoGetter.LoadCountriesTree(); } void Ranker::MakeRankerResults() { + LOG(LDEBUG, ("PreRankerResults number =", m_preRankerResults.size())); + bool const isViewportMode = m_geocoderParams.m_mode == Mode::Viewport; RankerResultMaker maker(*this, m_dataSource, m_infoGetter, m_reverseGeocoder, m_geocoderParams); diff --git a/search/search_integration_tests/pre_ranker_test.cpp b/search/search_integration_tests/pre_ranker_test.cpp index e667c0ad64605..01a38d4f4a1f7 100644 --- a/search/search_integration_tests/pre_ranker_test.cpp +++ b/search/search_integration_tests/pre_ranker_test.cpp @@ -95,12 +95,6 @@ UNIT_CLASS_TEST(PreRankerTest, Smoke) m2::PointD const kPivot(0, 0); m2::RectD const kViewport(-5, -5, 5, 5); - /// @todo Well, I'm not sure that 50 results will have unique distances to pivot. - /// 7x7 grid is 49, so potentially it can be 51 (north and south) or (east and west). - /// But we should consider circle (ellipse) around pivot and I can't say, - /// how it goes in meters radius on integer mercator grid. - size_t constexpr kBatchSize = 50; - vector pois; for (int x = -5; x <= 5; ++x) { @@ -111,7 +105,7 @@ UNIT_CLASS_TEST(PreRankerTest, Smoke) } } - TEST_LESS(kBatchSize, pois.size(), ()); + size_t const batchSize = pois.size() / 2; auto mwmId = BuildCountry("Cafeland", [&](TestMwmBuilder & builder) { @@ -133,7 +127,7 @@ UNIT_CLASS_TEST(PreRankerTest, Smoke) params.m_viewport = kViewport; params.m_accuratePivotCenter = kPivot; params.m_scale = scales::GetUpperScale(); - params.m_everywhereBatchSize = kBatchSize; + params.m_everywhereBatchSize = batchSize; params.m_limit = pois.size(); params.m_viewportSearch = false; preRanker.Init(params); @@ -159,8 +153,8 @@ UNIT_CLASS_TEST(PreRankerTest, Smoke) TEST(ranker.Finished(), ()); size_t const count = results.size(); - // See todo comment above for details. - TEST(count == kBatchSize || count == kBatchSize + 1, (count)); + // Depends on std::shuffle, but lets keep 6% threshold. + TEST(count > batchSize*1.06 && count < batchSize*1.94, (count)); vector checked(pois.size()); for (size_t i = 0; i < count; ++i) diff --git a/search/search_quality/search_quality_tests/real_mwm_tests.cpp b/search/search_quality/search_quality_tests/real_mwm_tests.cpp index 42f7490e68c60..48f71352ddcc4 100644 --- a/search/search_quality/search_quality_tests/real_mwm_tests.cpp +++ b/search/search_quality/search_quality_tests/real_mwm_tests.cpp @@ -97,6 +97,14 @@ class MwmTestsFixture : public search::tests_support::SearchTestBase } } + static size_t CountClassifType(Range const & results, uint32_t type) + { + return std::count_if(results.begin(), results.end(), [type](search::Result const & r) + { + return EqualClassifType(r.GetFeatureType(), type); + }); + } + static void NameStartsWith(Range const & results, base::StringIL const & prefixes) { for (auto const & r : results) @@ -335,7 +343,10 @@ UNIT_CLASS_TEST(MwmTestsFixture, Barcelona_Carrers) auto const & results = request->Results(); TEST_GREATER(results.size(), kTopPoiResultsCount, ()); - Range const range(results, 0, 4); + // - First 2 streets in Barcelona (1-2 km) + // - Next streets in Badalona, Barbera, Sadabel, ... (20-30 km) + // - Again 2 _minor_ footways in Barcelona (1-2 km) + Range const range(results, 0, 2); EqualClassifType(range, GetClassifTypes({{"highway"}})); CenterInRect(range, {2.1651583, 41.3899995, 2.1863021, 41.4060494}); } @@ -473,6 +484,7 @@ UNIT_CLASS_TEST(MwmTestsFixture, French_StopWord_Category) UNIT_CLASS_TEST(MwmTestsFixture, Street_BusStop) { // Buenos Aires + // Also should download Argentina_Santa Fe. ms::LatLon const center(-34.60655, -58.43566); SetViewportAndLoadMaps(center); @@ -481,9 +493,10 @@ UNIT_CLASS_TEST(MwmTestsFixture, Street_BusStop) auto const & results = request->Results(); TEST_GREATER(results.size(), kTopPoiResultsCount, ()); - // Top results are Hotel and Street (sometimes bus stop). + // Top results are Hotel, Shop and Street. + // Full Match street (20 km) is better than Full Prefix bus stop (1 km). Range const range(results); - EqualClassifType(range, GetClassifTypes({{"tourism", "hotel"}, {"highway", "bus_stop"}, {"highway", "residential"}})); + EqualClassifType(range, GetClassifTypes({{"tourism", "hotel"}, {"shop"}, {"highway", "residential"}})); } { @@ -824,4 +837,19 @@ UNIT_CLASS_TEST(MwmTestsFixture, BA_LasHeras) } } +UNIT_CLASS_TEST(MwmTestsFixture, BA_SanMartin) +{ + // Buenos Aires (Palermo) + ms::LatLon const center(-34.5801392, -58.415764); + SetViewportAndLoadMaps(center); + + { + auto request = MakeRequest("San Martin"); + auto const & results = request->Results(); + size_t constexpr kResultsCount = 12; + TEST_GREATER(results.size(), kResultsCount, ()); + TEST_GREATER(CountClassifType(Range(results, 0, kResultsCount), + classif().GetTypeByPath({"railway", "station"})), 2, ()); + } +} } // namespace real_mwm_tests