Sort locations in timetable by CmdrTMir · Pull Request #200 · motis-project/nigiri

CmdrTMir · 2025-05-21T11:08:42Z

Sortiert die locations in timetable, anhand der Anzahl der Routen pro location.

felixguendling · 2025-05-23T11:55:37Z

-    tt.location_areas_ = sorted_loc_area;
-    tt.route_location_seq_ = sorted_route_loc_seq;
+
+    tt.permutate_locations(first_idx);


has to be in init_finish (and probably better optional)

felixguendling · 2025-05-23T11:56:21Z

 static vector<location_idx_t> mapping_vec;

-vector<location_idx_t> create_mapping_vec(vector<location_idx_t> vec) 
+static vector<location_idx_t> create_mapping_vec(vector<location_idx_t> vec) 


move to .cc file

(if in header, then use inline instead of static but if not a template and not performance sensitive -> move to .cc file)

felixguendling · 2025-05-23T11:58:02Z

+static vector<location_idx_t> mapping_vec;
+
+static vector<location_idx_t> create_mapping_vec(vector<location_idx_t> vec) 
+{


always use formatter (format on save tool in Clion, but every editor has support for this)

clang-format-18

felixguendling · 2025-05-23T11:58:23Z

+
+
+template<typename T>
+T apply_permutation_vec(T input) 


always const&

felixguendling · 2025-05-23T11:58:55Z

+  return result_vec;
+}
+
+static void build_permutation_vec(vecvec<location_idx_t, route_idx_t> order, uint32_t first_idx) 


always const&

https://github.com/motis-project/motis/blob/master/docs/STYLE.md

felixguendling · 2025-05-30T16:08:22Z

+vector<location_idx_t> permutation_;
+static vector<location_idx_t> mapping_vec;


no global variables

felixguendling · 2025-05-30T16:08:36Z

  }
+  if (opt.permutate_loc_) {
+    auto first_idx_size_t = special_stations_names.size();
+    uint32_t first_idx = static_cast<uint32_t>(first_idx_size_t);


Suggested change

uint32_t first_idx = static_cast<uint32_t>(first_idx_size_t);

auto const first_idx = static_cast<std::uint32_t>(first_idx_size_t);

felixguendling · 2025-05-30T16:08:45Z

  auto const progress_tracker = utl::get_active_progress_tracker();
  auto timezones = tz_map{};
  auto agencies = read_agencies(tt, timezones, load(kAgencyFile).data());
+  // stops = hash_map<string(id), location_idx_t>


felixguendling · 2025-05-30T16:09:32Z

+    auto sorted_loc_routes = apply_permutation_vec(location_routes_);
+    auto sorted_names = apply_permutation_vec(locations_.names_);
+    auto sorted_ids = apply_permutation_vec(locations_.ids_);
+    auto sorted_coords = apply_permutation_vec(locations_.coordinates_);
+    auto sorted_src = apply_permutation_vec(locations_.src_);
+    auto sorted_transfertime = apply_permutation_vec(locations_.transfer_time_);
+    auto sorted_types = apply_permutation_vec(locations_.types_);
+    auto sorted_timezones =
+        apply_permutation_vec(locations_.location_timezones_);
+    auto sorted_parents =
+        apply_permutation_and_mapping_vec(locations_.parents_);
+    auto sorted_equivalences =
+        apply_permutation_multimap(locations_.equivalences_);
+    auto sorted_children = apply_permutation_multimap(locations_.children_);
+    auto sorted_pre_footpaths_out = apply_permutation_multimap_foot(
+        locations_.preprocessing_footpaths_out_);
+    auto sorted_pre_footpaths_in =
+        apply_permutation_multimap_foot(locations_.preprocessing_footpaths_in_);
+    auto sorted_loc_area = apply_permutation_vec(location_areas_);
+    auto sorted_route_loc_seq =
+        apply_permutation_to_route_loc_seq(route_location_seq_);
+
+    locations_.location_id_to_idx_ = std::move(sorted_loc_id_to_idx);
+    locations_.names_ = std::move(sorted_names);
+    locations_.ids_ = std::move(sorted_ids);
+    locations_.coordinates_ = std::move(sorted_coords);
+    locations_.src_ = std::move(sorted_src);
+    locations_.transfer_time_ = std::move(sorted_transfertime);
+    locations_.types_ = std::move(sorted_types);
+    locations_.location_timezones_ = std::move(sorted_timezones);
+    locations_.parents_ = std::move(sorted_parents);
+    locations_.equivalences_ = std::move(sorted_equivalences);
+    locations_.children_ = std::move(sorted_children);
+    locations_.preprocessing_footpaths_out_ =
+        std::move(sorted_pre_footpaths_out);
+    locations_.preprocessing_footpaths_in_ = std::move(sorted_pre_footpaths_in);
+    location_areas_ = std::move(sorted_loc_area);
+    route_location_seq_ = std::move(sorted_route_loc_seq);
+    location_routes_ = std::move(sorted_loc_routes);


why variables and not just assignments?

felixguendling · 2025-05-30T16:09:56Z

+    vector<location_idx_t> location_permutation =
+        build_permutation_vec(location_routes_, first_idx);
+
+    vector<std::pair<location_id, location_idx_t>> unsorted;


AAA style mentioned in https://github.com/motis-project/motis/blob/master/docs/STYLE.md

felixguendling · 2025-05-30T16:10:26Z

+
+template <typename T>
+inline T apply_permutation_vec(T const& input) {
+  T sorted;


Suggested change

T sorted;

auto sorted = T{};

felixguendling · 2025-05-30T16:10:49Z

+inline T apply_permutation_vec(T const& input) {
+  T sorted;
+  for (auto i = 0U; i < input.size(); ++i) {
+    auto temp = input.at(permutation_[i]);


Suggested change

auto temp = input.at(permutation_[i]);

auto temp = input[permutation_[i]];

no check necessary

felixguendling · 2025-05-30T16:11:08Z

+vecvec<route_idx_t, stop::value_type> apply_permutation_to_route_loc_seq(
+    vecvec<route_idx_t, stop::value_type> const& input);
+
+vector_map<location_idx_t, location_idx_t> apply_permutation_and_mapping_vec(
+    vector_map<location_idx_t, location_idx_t> const& input);
+
+mutable_fws_multimap<location_idx_t, location_idx_t> apply_permutation_multimap(
+    mutable_fws_multimap<location_idx_t, location_idx_t> const& input);
+
+mutable_fws_multimap<location_idx_t, footpath> apply_permutation_multimap_foot(
+    mutable_fws_multimap<location_idx_t, footpath> const& input);


why in header? only used locally in .cc file

felixguendling · 2025-05-30T16:11:24Z

+    vecvec<location_idx_t, route_idx_t> const& order, uint32_t const first_idx);
+
+template <typename T>
+inline T apply_permutation_vec(T const& input) {


why in header? only used locally in .cc file

felixguendling · 2025-05-30T16:15:04Z

  void write(std::filesystem::path const&) const;
  static cista::wrapped<timetable> read(std::filesystem::path const&);

+  void permutate_locations(uint32_t first_idx) {


can be a separate function in a separate file

permutate_locations.h: permutate_locations(timetable&);
permutate_locations.cc: permutate_locations(timetable&) { ... }

init_finish() { ... permutate_locations(tt); }

felixguendling · 2025-06-02T13:56:23Z

  bool adjust_footpaths_{true};
  bool merge_dupes_intra_src_{true};
  bool merge_dupes_inter_src_{true};
+  bool permutate_loc_{true};


does it have any downsides? if no, no option needed

siehe E-Mail

felixguendling · 2025-06-02T13:56:41Z

              bool adjust_footpaths = false,
              bool merge_dupes_intra_src = false,
              bool merge_dupes_inter_src = false,
+              bool permutate_loc = false,


does it have any downsides? if no, no option needed?

felixguendling · 2025-06-02T13:56:55Z

+#include "nigiri/timetable.h"
+#include "nigiri/types.h"
+
+#include <algorithm>


include order (common -> specific)

felixguendling · 2025-06-02T13:57:40Z

+  if (opt.permutate_loc_) {
+    auto first_idx_size_t = special_stations_names.size();
+    auto const first_idx = static_cast<std::uint32_t>(first_idx_size_t);
+    permutate_locations(tt, first_idx);


first idx can be a constant? it's a detail anyway, no need to have it do the caller?

felixguendling · 2025-06-02T13:58:22Z

+  for (auto i = 0U; i < n; ++i) {
+    result[permutation[i].v_] = location_idx_t{i};
+  }
+  vector<location_idx_t> mapping_vec;


left side = auto

please fix the remaining places also

Suggested change

vector<location_idx_t> mapping_vec;

auto mapping_vec = vector<location_idx_t>{};

felixguendling · 2025-06-02T13:58:58Z

+
+namespace nigiri {
+
+auto create_mapping_vec(vector<location_idx_t> const& permutation) {


Suggested change

auto create_mapping_vec(vector<location_idx_t> const& permutation) {

std::vector<location_idx_t> create_mapping_vec(vector<location_idx_t> const& permutation) {

explicit return types = better readability

felixguendling · 2025-06-02T13:59:57Z

+    vector<location_idx_t> mapping_vec) {
+  vector_map<location_idx_t, location_idx_t> sorted;
+  for (auto i = 0U; i < input.size(); ++i) {
+    auto temp = input.at(permutation[i]);


Suggested change

auto temp = input.at(permutation[i]);

auto temp = input[permutation[i]];

we should be 100% sure that permutation[i] is within bounds, right?

felixguendling · 2025-06-02T14:00:42Z

+
+auto apply_permutation_and_mapping_vec(
+    vector_map<location_idx_t, location_idx_t> const& input,
+    vector<location_idx_t> permutation,


Suggested change

vector<location_idx_t> permutation,

vector<location_idx_t> permutation,

never pass vectors by value if you don't actually need a copy. why copy here?
applies everywhere!

CmdrTMir and others added 9 commits April 28, 2025 17:21

load timetable sortieren

ba8e171

changes to load timetable for permutating locations

1a1a29e

Merge branch 'motis-project:master' into sort-loc

67b5367

permutate location in .h file

0597829

Merge branch 'motis-project:master' into sort-loc

1f6ec16

Merge branch 'motis-project:master' into sort-loc

76f0871

funktion ausgelagert und nicht gebrauchte funktion gelöscht

13ab3a8

Merge branch 'motis-project:master' into sort-loc

5c07cb4

Merge branch 'sort-loc' of github.com:CmdrTMir/nigiri into sort-loc

1f00aed

felixguendling reviewed May 23, 2025

View reviewed changes

CmdrTMir added 7 commits May 23, 2025 14:03

clang-format

3b3b828

ausgelagert in .cc Datei

5fb4faf

move to init finish

d300a8b

timetable update

d7b7242

timetable changes

cc859bf

Merge branch 'master' of github.com:CmdrTMir/nigiri into sort-loc

b406486

format

fb2b28c

felixguendling reviewed May 30, 2025

View reviewed changes

felixguendling force-pushed the master branch from 96bc808 to 89b9084 Compare June 1, 2025 09:06

CmdrTMir added 2 commits June 2, 2025 15:18

implement suggested changes

87053c6

Merge branch 'master' of github.com:CmdrTMir/nigiri into sort-loc

bc9a26a

felixguendling requested changes Jun 2, 2025

View reviewed changes

CmdrTMir and others added 4 commits June 11, 2025 13:07

implement requested changes

02fded5

fix first index

5f7e0aa

permutate locations optional (=true)

86a79b6

refactor, add fields, add disclaimer, make reorder default

0b32292

felixguendling added 4 commits July 13, 2025 21:06

remove unused header, formatting

c84cfea

Merge branch 'master' of github.com:motis-project/nigiri into sort-loc

b9ff4c2

update cista

8135e26

formatting

dec68d2

felixguendling force-pushed the master branch from 549a8d9 to 5d5eb32 Compare September 18, 2025 10:32

felixguendling force-pushed the master branch 2 times, most recently from a4efa76 to 116082c Compare December 11, 2025 14:31

		vector<location_idx_t> permutation_;
		static vector<location_idx_t> mapping_vec;

	uint32_t first_idx = static_cast<uint32_t>(first_idx_size_t);
	auto const first_idx = static_cast<std::uint32_t>(first_idx_size_t);

	auto temp = input.at(permutation_[i]);
	auto temp = input[permutation_[i]];

	vector<location_idx_t> mapping_vec;
	auto mapping_vec = vector<location_idx_t>{};


		namespace nigiri {

		auto create_mapping_vec(vector<location_idx_t> const& permutation) {

	auto create_mapping_vec(vector<location_idx_t> const& permutation) {
	std::vector<location_idx_t> create_mapping_vec(vector<location_idx_t> const& permutation) {

	auto temp = input.at(permutation[i]);
	auto temp = input[permutation[i]];

	vector<location_idx_t> permutation,
	vector<location_idx_t> permutation,

Uh oh!

Conversation

CmdrTMir commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants