[search][ranking] POIs ranking from scratch. #3035
No reviewers
Labels
No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
No milestone
No project
No assignees
1 participant
No due date set.
Dependencies
No dependencies set.
Reference: organicmaps/organicmaps-tmp#3035
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "vng-dev"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Affected issues:
Closes organicmaps/organicmaps#3026
Closes organicmaps/organicmaps#2530
Closes organicmaps/organicmaps#2470
Closes organicmaps/organicmaps#2133
Closes organicmaps/organicmaps#2070
Closes organicmaps/organicmaps#1997
Closes organicmaps/organicmaps#1949
Closes organicmaps/organicmaps#1560
Closes organicmaps/organicmaps#1376
Closes organicmaps/organicmaps#678
FAILED
search_integration_tests/processor_test.cpp:2417 TEST(results[1].GetRankingInfo().GetLinearModelRank() > results[0].GetRankingInfo().GetLinearModelRank()) 0.104256 0.106698
Предлагаю быстро вынести из этого PR простые и очевидные коммиты с мелкими улучшениями и переписываниями кода, и быстро их помержить. Чтобы PR не разростался.
@ -359,0 +340,4 @@{"motorway", Motorway},{"motorway_link", Motorway},{"path", Outdoor},{"pedestrian", Pedestrian},Больше нет зависимости от категорий?
@ -215,1 +216,4 @@DECLARE_CHECKER_INSTANCE(IsWayChecker);enum SearchRank : uint8_t{Да, переименуй, чтобы с осмом не путать.
?
Может хранить их как constexpr string_view для ускорения инициализации и избежания ненужных аллокаций далее?
@ -328,8 +328,19 @@ private:// Bulgarian - Български"булевард", "бул", "площад", "пл", "улица", "ул", "квартал", "кв",В этом списке есть дубликаты. Они фильтруются? Можно хотя бы руками явно закомментить. "бул" например.
string_view?
Может string_view таки?
Это точно не нужно? Комментарий бы хотя бы.
Зачем тут фигурные скобки?
И в других местах ниже тоже, не по стилю же.
Семантику вызова с nullptr хорошо бы задокументировать в комментарии.
@ -86,4 +83,3 @@unique_ptr<RankTable> ratings = make_unique<DummyRankTable>();unique_ptr<LazyCentersTable> centers;bool pivotFeaturesInitialized = false;Не надо тут reserve()?
@ -219,2 +195,2 @@// to small changes in the original set.shuffle(b, e, m_rng);nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessRankAndPopularity, m_results));filtered.insert(iBeg, iMiddle);Может стоит вынести m_results.begin() и end() в переменные?
Кстати, вспомнил, что в С++17 порядок вычисления аргументов функции строго гарантирован слева направо, но ты наверное это знаешь :)
Вообще, конечно, хорошая реализация поиска продолжит тихонько лупить в фоне после выдачи первоначального лимита результатов, чтобы можно было скроллить дальше. Ну или хотя бы будет дальше запускать поиск явно, если юзер доскроллит в низ списка результатов.
Это мог быть какой-то edge-case, когда движок не инициализируется нормально, например (протести!) вызов поиска из апишки. Но его нужно тогда системно исправлять, а не ненужной проверкой тут, конечно же.
small_set? Тут же динамическая аллокация на каждый айдишник, верно?
Это означает, что наш лимит на количество фич, выданных поиском, может на самом деле быть больше, верно? kMax + m_currEmit.size() ?
Потому что если нет, то будет моргать. Или я не прав?
Так вот почему плохо ищутся деревни )
Инициализировать точно не нужно по-умолчанию?
m_purrCats :)
Уже лучше тогда полностью слово )
Фальшивые коты! 🤣
@ -105,15 +97,30 @@ ErrorsMade GetPrefixErrorsMade(QueryParams::Token const & token, strings::UniStrbool IsStopWord(UniString const & s)Тут же явно не все языки. Откуда взялись эти строки?
И уточняющий вопрос: если юзер вбил явно "de La Feature", у него 1 к одному заматчится и будет в топе эта фича? Или стопворды заматчат только Feature?
const
@ -141,8 +149,12 @@ string DebugPrint(NameScore const & score)string DebugPrint(NameScores const & scores)Давно мучаюсь вопросом: можно ли заменить наш UniString на char32_t? Тогда можно было бы создавать при компиляции constexpr строки
U"это будет unicode 32 строка"и избегать рантайм оверхеда.Хочется, чтобы у всех юзеров ОМ запускался мгновенно и работал везде и во всём быстрее всех, судя по всему, это одна из наших killer-features сейчас )
@ -548,2 +544,3 @@auto countryId = BuildCountry(countryName, [&](TestMwmBuilder & builder) {auto countryId = BuildCountry("Wonderland", [&](TestMwmBuilder & builder){builder.Add(street);Полотенца тестов получаются, может удобнее было бы builder.Add(cafe1).Add(cafe2).Add(cafe3)? Или что-то вроде builder.Add({cafe1, cafe2, cafe3})?
@ -655,3 +649,4 @@});// Tests that postcode is added to the search index.{kWonderland = "Wonderland"?
@ -660,8 +655,9 @@ UNIT_CLASS_TEST(ProcessorTest, TestPostcodes)QueryParams params;const?
@ -808,4 +802,2 @@string const countryName = "Wonderland";TestCity sanDiego({0, 0}, "San Diego", "en", 100 /* rank */);TestCity homel({10, 10}, "Homel", "en", 100 /* rank */);Если тут одно и то же везде, то лучше сразу BuildWonderland(lambda).
@ -82,0 +71,4 @@test("San Francisco", "San ", TokenRange(0, 1), NameScore::FULL_PREFIX, 0, 3);test("San Francisco", "san fr", TokenRange(0, 2), NameScore::PREFIX, 0, 5);test("San Francisco", "san fracis", TokenRange(0, 2), NameScore::PREFIX, 1, 9);test("South Fredrick Street", "S Fredrick St", TokenRange(0, 3), NameScore::FULL_MATCH, 0, 11);Если весь тест внутри своего namespace, можно анонимные внутри и не делать, зачем?
Кажется тут скоуп не нужен )
А тут зачем скоуп? :)
@ -359,0 +340,4 @@{"motorway", Motorway},{"motorway_link", Motorway},{"path", Outdoor},{"pedestrian", Pedestrian},Есть категории, есть улицы. Можно зависимость искать, можно нет. Нет смысла тут натягивать.
@ -328,8 +328,19 @@ private:// Bulgarian - Български"булевард", "бул", "площад", "пл", "улица", "ул", "квартал", "кв",Дубликаты не страшны
Зачем?
Они хранятся как UniString в итоге
зачем?
Неиспользуемый код. Все же собирается :)
default = 0 initialization
@ -86,4 +83,3 @@unique_ptr<RankTable> ratings = make_unique<DummyRankTable>();unique_ptr<LazyCentersTable> centers;bool pivotFeaturesInitialized = false;там неизвестно кол-во, в целом можно - да, можно - нет
@ -219,2 +195,2 @@// to small changes in the original set.shuffle(b, e, m_rng);nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessRankAndPopularity, m_results));filtered.insert(iBeg, iMiddle);можно, а что с порядком не так?
@ -219,2 +195,2 @@// to small changes in the original set.shuffle(b, e, m_rng);nth_element(iBeg, iMiddle, iEnd, CompareIndices(&PreRankerResult::LessRankAndPopularity, m_results));filtered.insert(iBeg, iMiddle);она так и работает, и это хорошо и не очень одновременно
протести, assert покажет
он тут не small, возможно (не уверен) стоит заменить на unordered
Тут не про лимит. Храним предидущий выхлоп, чтобы на следующем он тоже по максимуму остался.
нет
в конструкторе, для битов = семантика не работает
purr?
@ -105,15 +97,30 @@ ErrorsMade GetPrefixErrorsMade(QueryParams::Token const & token, strings::UniStrbool IsStopWord(UniString const & s)@ -141,8 +149,12 @@ string DebugPrint(NameScore const & score)string DebugPrint(NameScores const & scores)Мы всегда делаем NormalizeAndSimplify, потому constexpr тут не будет, а тем более в set
@ -655,3 +649,4 @@});// Tests that postcode is added to the search index.{зачем?
предполагается что тесты будут добавляться
предполагается что тесты будут добавляться
@ -548,2 +544,3 @@auto countryId = BuildCountry(countryName, [&](TestMwmBuilder & builder) {auto countryId = BuildCountry("Wonderland", [&](TestMwmBuilder & builder){builder.Add(street);мне не удобнее
тут код - лучший комментарий :) есть - передай, нет - не передавай
А могли бы храниться как std::u32string_view
Нифига не очевидно, это же enum. Напиши лучше явно
@ -105,15 +97,30 @@ ErrorsMade GetPrefixErrorsMade(QueryParams::Token const & token, strings::UniStrbool IsStopWord(UniString const & s)@ -141,8 +149,12 @@ string DebugPrint(NameScore const & score)string DebugPrint(NameScores const & scores)Скорее всего нормализацию можно сделать compile-time constexpr при желании )
Там все равно вызывается нормализация. Ну нет тут практической выгоды
enum определяется в cpp, вынесу в конструктор
@ -105,15 +97,30 @@ ErrorsMade GetPrefixErrorsMade(QueryParams::Token const & token, strings::UniStrbool IsStopWord(UniString const & s)Это можно экспериментировать потом. Но вообще логика скипания stopword в поиске вполне стандартная.
@ -141,8 +149,12 @@ string DebugPrint(NameScore const & score)string DebugPrint(NameScores const & scores)Вот так жизнь зря и проходит в войне с мельницами :)
@ -141,8 +149,12 @@ string DebugPrint(NameScore const & score)string DebugPrint(NameScores const & scores)Кто-то интегралы считал шаблонами во время компиляции, когда constexpr ещё не было )
@ -252,0 +204,4 @@comparator.m_positionIsInsideViewport =m_params.m_position && m_params.m_viewport.IsPointInside(*m_params.m_position);comparator.m_detailedScale = mercator::DistanceOnEarth(m_params.m_viewport.LeftTop(),m_params.m_viewport.RightBottom()) <Выше шаффлится чтобы был random subset of lower priority results. Оно нам точно надо?
Тут получается всего 3iMiddle2 копирований (мув в конце на самом деле копирует, там нечего мувать).
Если в сете будут не сами результаты, а указатели на vector результатов, то копирований будет в два раза меньше.
Если реально фиксятся ишью по ссылкам в описании, поправь его сразу чтобы там было Fixes #xxx
@ -252,0 +204,4 @@comparator.m_positionIsInsideViewport =m_params.m_position && m_params.m_viewport.IsPointInside(*m_params.m_position);comparator.m_detailedScale = mercator::DistanceOnEarth(m_params.m_viewport.LeftTop(),m_params.m_viewport.RightBottom()) <не могу сказать определенно, по комментам звучит разумно, потом можно экспериментировать
Они фиксятся, но я не хочу чтобы issues автоматом (наверное?) закрылись, это для меня самого пометки на потом для написания тестов
В итоге не факт что и сколько будет лучше, но PR не об этих спичках, помержим, бери код и делай эксперименты.
@ -252,0 +204,4 @@comparator.m_positionIsInsideViewport =m_params.m_position && m_params.m_viewport.IsPointInside(*m_params.m_position);comparator.m_detailedScale = mercator::DistanceOnEarth(m_params.m_viewport.LeftTop(),m_params.m_viewport.RightBottom()) <Другое дело, что там идет шаффл для результатов, которые точно равны по расстоянию. Врядли мы будем иметь хоть несколько результатов, которые double: distM1 == distM2
Если уж делать, то для интервала с каким-то допуском ..
Вот что получилось: organicmaps/organicmaps#3035/commits/dd6857db9fc84443023697a2cb454f0dc0247e4d
@ -252,0 +204,4 @@comparator.m_positionIsInsideViewport =m_params.m_position && m_params.m_viewport.IsPointInside(*m_params.m_position);comparator.m_detailedScale = mercator::DistanceOnEarth(m_params.m_viewport.LeftTop(),m_params.m_viewport.RightBottom()) <О, видишь, а я это недоглядел. Конечно надо поправить. Давай попробуем может вообще без этого шафла?
Выглядит вроде норм, юнит тест бы мелкий.
Он есть, там небольшой вопрос по порядку исходных элементов nth_element ибо в этом искусственном тесте генерятся куча пои с уже одинаковым расстоянием :)
@ -252,0 +204,4 @@comparator.m_positionIsInsideViewport =m_params.m_position && m_params.m_viewport.IsPointInside(*m_params.m_position);comparator.m_detailedScale = mercator::DistanceOnEarth(m_params.m_viewport.LeftTop(),m_params.m_viewport.RightBottom()) <Я уже добавил правки с ассертом в Review fixes комит.
Ох накодякал... Будет быстрее если правки поверх накинешь, и перед мержем сольёшь.
Почти готово, надо только погонять на девайсе перед мержем.
@ -28,2 +25,4 @@{using namespace std;template <class ContT> string TypesToString(ContT const & holder)Креатор ещё и неюзаемые неймспейсы детектит?
const?
const &
Не заметил вторую скобку в конце.
Если порядок айдишников не важен, то конечно замени на unordered.
Я вот твои iff не понимаю. Чем бы отличался смысл, если бы тут была одна буква f?
const
Что-то ты часто стал пропускать. А тут будет много оверхеда без const.
@ -241,1 +265,4 @@result += kPoiType[base::Underlying(m_classifType.poi)];}else if (m_type == Model::TYPE_STREET){ASSERT не прокатит?
фыркающие коты )
нет, нельзя
тут не понял
Эти комменты были и писались не мной
@ -28,2 +25,4 @@{using namespace std;template <class ContT> string TypesToString(ContT const & holder)нет
@ -241,1 +265,4 @@result += kPoiType[base::Underlying(m_classifType.poi)];}else if (m_type == Model::TYPE_STREET){assert и check разница понятна, выбор всегда на "а чего хотим"
Оставить чек, чтобы потенциальный вылет был читаем, а не стрельба в память
отличная пасхалочка :)
Хэш точно будет так эффективнее, чем сет ранее?
Кривое форматирование было.
В целом unordered эффективнее чем set на не очень маленьких размерах. Несмотря на вроде-бы тяжелый hash, лучше он чем алоцировать память на каждую ноду. Не говоря уже про сложность.
Хм, в этом кейзе нет. Но const важен для чтения кода.
Ты запускаешь интеграционные тесты перед каждым новым релизом данных?
It's
Мы умеем отличать
cafeотcafe? Во втором случае юзер вполне может ожидать Cafe Pushkin.А мы можем вываливать какие-то специальные подсказки/саджесты для юзера, когда наматчит и того и того, чтобы например уточнить, он ищет только категории (все магазины), или по названию (конкретные имена пои, не обязательно магазины)?
@ -0,0 +63,4 @@static std::vector<uint32_t> GetClassifTypes(std::vector<base::StringIL> const & arr){std::vector<uint32_t> res;С++17 умеет тут auto? Или только если выше, в функции?
Это не мой коммент, тут остается без изменений.
Здесь не про это. Тут все магазины, но без фикса по запросу "rewe", магазин "REWE City" будет далеко внизу даже если он рядом, потому что все забьет точный матчинг из магазинов с именем "REWE".
@ -0,0 +63,4 @@static std::vector<uint32_t> GetClassifTypes(std::vector<base::StringIL> const & arr){std::vector<uint32_t> res;auto res; И выводит тип из конструкции return? Сомневаюсь :)
Я тесты только добавил и буду запускать каким-то скриптом после генерации данных вместе с routing_integration_tests.
@ -0,0 +63,4 @@static std::vector<uint32_t> GetClassifTypes(std::vector<base::StringIL> const & arr){std::vector<uint32_t> res;Да, только наоборот можно.