When running the docker image on OSX , the documentation does not get rendered for ?movetk_core::MakePoint.

The FreespaceDiagram defined in StrongFrechetDistance should perhaps reside in the movetk/ds folder, next to the other, non-parameterized, FSD. In addition, there is some duplication that can be removed with some refactoring.

Currently, we are not running the tests as part of CI, which would help catching errors earlier.

Vanilla Fedora image does not build.
Python dependency is not part of image.
After skipping this, radpidjson header files are then missing

The workflow for creation of docker image fails with the following error

/opt/conda/bin/../lib/gcc/x86_64-conda_cos6-linux-gnu/7.3.0/../../../../x86_64-conda_cos6-linux-gnu/bin/ld: warning: libwebp.so.7, needed by /opt/conda/lib/libgdal.so, not found (try using -rpath or -rpath-link)```

• Docker image
• rpm & debian packages

There is some existing code that is commented out, Usually unused typedefs and cout calls. For code quality, I suggest these be removed.

Currently, the CI documentation build process (in the docs folder) causes problems with merging. Need to figure out a different process.

Also investigate if we can use it for MSVC (seems to be support).

To avoid possible clashes with other libraries, I think it would be nice if we prefix MoveTk-specific CMake variables/ preprocessor variables with a common suffix such as MOVETK_. For me as a CMake gui user, this has the additional benefit that all MoveTk related CMake variables would be grouped.

Would it make sense to construct a geographic/geodetic kernel, where we also support conversion between coordinate systems? We could encapsulate the coordinate transformations there instead of having separate functions that either wrap GeographicLib or perform somewhat simplified conversions. In addition, some algorithms may also work for non-Euclidean space.

Create modules with diverse dependencies and a bare bone code base for systems missing larger libraries

Currently, in our Windows CI, we pull in the entirety of boost, which makes the current Windows CI process quite slow (80+ minutes). I think if we use a more finegrained selection of the boost packages, this should reduce the depedencies download and build time in CI

In our kernels, we name everything prefixed with Movetk e.g. MovetkPoint, MovetkLine. This prefix, however, does not really add any value. Would it make sense to simply rename them to Point, Line etc.? This makes using movetk with user defined kernels perhaps easier, since they do not need to use the prefix.

In addition, it may be convenient to add aliases for the geometric objects in the movetk::geom namespace, e.g.

template<typename Kernel>
using Point = typename Kernel::Point; //or MovetkPoint, if we decide not to remove the prefix

This makes using these types easier, then you can do movetk::geom::Point<Kernel> instead of typename Kernel::Point. This is similar to how CGAL approaches geometric objects.

Currently, Distances.h is directly included in the GeometryInterface.h header. This disallows any algorithms in Distances.h from reusing code such as the MakeSth objects in GeometryInterface.h. Decoupling this dependency would be beneficial to achieve more code reuse I think.

@aniketmitra001 already made a design for decoupling

The following line of code may assert unnecessarily:

https://github.com/heremaps/movetk/blob/36d3deea5b5283d4775ba35d256fbe02cdd049ba/src/include/movetk/algo/Interpolation.h#L281

Is it needed? What is it asserting to be true?

MoveTK is a geometry backend agnostic library. This means that the choice of the dimension, coordinate type and geometry library (Boost.Geometry or CGAL) is delegated to the application developer. Since, in the library these types are template parameters, the compiler expects these types to be defined while building the project. While writing python bindings with PyBind11, this means that the types have to be defined in the .cpp file. Therefore in the binding code , we will need to define, BoostGeometryKernel2D, BoostGeometryKernel3D, CGALGeometryKernel2D,... So the user if the python module will be restricted by the choice of the dimension and geometry backend defined in the binding code. Moreover we can limit the number type to long double. So the trade off with ease of use seems to come at the cost of lack of flexibility. We need to check if there can be a workaround to this

The docker file in docker/with_jupyterlab uses the cmake installed from the package manager of fedora to build MoveTK
On the other hand xeus-cling required for running the C++ tutorials in a Jupyter notebook is installed with Miniconda.

This can be confusing. Its better to build MoveTK with cmake and dependencies installed from Miniconda

Maybe this is more of a stylistic question: currently, our namespaces are all prefixed by movetk_ and do not map directly to the folders that the associated methods/classes are in. I personally prefer to synchronize the names of the folders of the (includable) header files with the namespaces that the objects reside in. Using this guideline, it is easier to see what you need to include from a fully qualified name, e.g. movetk::core::MakePoint<> would then reside in some file in 'movetk/core' in the header include folder. Is this something we would want to pursue?

vcpkg supports features out-of-the-box, where we can give names to certain dependency sets( see this). It would be nice to use this to split up the dependencies for the different backends, as well as dependencies for building the examples/tests/documentation?

conan can do something similar via options, but then we have to convert the conanfile.txt to a conanfile.py file and implement it( see this)

Hi Bram

wrt the the following changes in Distances.h

//Use std::function, otherwise boost will error when trying to copy the transform_iterator
std::function<NT(const typename Kernel::MovetkPoint &)> transform = [poly_a](const auto &polyBEl) {
                     SqDistance sqDist;
                     return std::sqrt(sqDist(*poly_a, polyBEl));
                 };

auto maxElIt = std::max_element(boost::make_transform_iterator(poly_b, transform), boost::make_transform_iterator(poly_b_beyond, transform));

This fails to compile on some versions of clang as std::max_element expects an iterators that is a model of a ForwardIterator and boost::make_transform_iterator seems to model an InputIterator

Please see this stack overflow discussion.

As suggested in the discussion of SO, we could return by reference from the lambda function but doesn't seem to be advisable. Please see the notes here

For the lack of a better alternative I have had to substitute the above with the following suboptimal piece of code

  std::vector<NT> distances;
                std::transform(poly_b, poly_b_beyond, std::back_insert_iterator(distances),
                               [poly_a](const auto &polyBEl) {
                                   SqDistance sqDist;
                                   return std::sqrt(sqDist(*poly_a, polyBEl));
                               });
                auto maxElIt = std::max_element(std::begin(distances), std::end(distances));

in this commit d0e8b27

With the TrajectoryReader we can read trajectories from probes, possibly removing duplicate probes by for example the dates. Should we use a stable sorting algorithm before that to make sure the outcome is the same if we use the same probe data file? We now use std::sort, which does not guarantee this. We maybe could use std::stable_sort instead?

GDAL is a heavyweight dependency for MoveTk, and a quick scan of the codebase shows that we only use it for a polygon reader and generic converters for iterators to GDAL data types. It seems to me like a user should be able to decide for themselves whether they want to use GDAL for this functionality, especially since it is heavy weight, hence we could remove the dependency and glue code. Alternatively, we could implement a kernel based on GDAL instead, but I'm not entirely sure this has a lot of added value.

MEOS is a C++ library which makes it easy to work with temporal and spatiotemporal data . It is based on MobilityDB's data types and functions.

Add support for the trajectory objects of MobilityDB using MEOS's C++ API

GA automatically deletes the cache if it hasn't been used for a week. Especially for the dependencies, this makes the build times of the CI increase, while our dependencies do not often change. See if we can construct a script that runs once a week to keep this cache alive.

Only support this if a sensible conversion is possible. This may make it easier to support some algorithms that are easier implementable (or implementable at all) in another kernel.

It should be ProbeParseDate instead of std::string

Hi Bram

I see that in your latest commits you have disabled the setting of the first value on the output iterator for the above functions

//*iter = 0;

with the following comment

Add a less strict input iterator requirement. Add point iterators to trajectory utils and make some function a bit less strict on the iterators

The idea of setting the first value to zero was to ensure that the size of the output is same as the input, as as we treat the result of movetk_core::get_time_diffs() as a column that can be added to the tabular trajectory data structure , so if the tabular trajectory has size m x n, then the output of movetk_core::get_time_diffs() is m x 1

Not setting the first value to 0 results in an output of size (m - 1) x 1

For N timestamps, get_time_diffs returns n-1 results. Should this be N, with the first value being 0?
For N positions, get_distances returns N results. Neither the first or last distance is 0. One of these should be incorrect.
Both methods should be consistent.

Calculation appears to be difference between N and N +1, hence It appears that the last distance calculation must be incorrect as there is no N + 1

However, a change in behaviour will break existing code, so more clarification in the API documentation is perhaps called for.

Create Python bindings for MoveTK

The current version we use does not build anymore on modern Linux due to changes in libstdc++. This is fixed upstream in Catch2, but requires us bumping the version. There are some breaking changes between v2.x and v3.x though, so we need to check what needs to change.

Currently, this does not work (see https://movetk.github.io/movetk/)

We may not want to always run this, but maybe every week/month or something like that. May also be interesting to be able to manually trigger it

The build fails on windows.

https://github.com/heremaps/movetk/actions/runs/249735823

The current GeoJSONUtil.h in movetk/io/ creates the GeoJSON structure by adding to a stream operator and is not very extendable.

Reported by @bacusters-tue

At present there are no concept-like definitions for the geometric primitives for the kernel
Right now the problem is lets say we are using the Boost Backend and in the wrapper for boost we have added an interface for adding two points , but for the the CGAL backend we haven’t , so when we parameterise an algorithm that adds two points and use the CGAL backend , we will get a compile time error. So if we define a set of requirements on the MovetkGeometryKernel (/src/include/movetk/geom/GeometryInterface.h) wherein for instance WrapperGeometryKernel::Wrapper_Point should satisfy those requirements, we will in a way be enforcing the homogeneity of the implementation in WrapperGeometryKernel.

The examples/include/GeometryBackendTraits.h header file is now supposed to be included in py/movetk_geometry.cpp. Also tutorials/TrivialGeometry.h is similar to examples/include/GeometryBackendTraits.h
So have one copy of GeometryBackendTraits.h and place it under src/include/movetk/utils

While you are at it move Geolife.h, GeolifeProbeTraits.h,GeolifeTrajectoryTraits.h, HereProbeTraits.h, Algorithms.h to src/include/movetk/utils as these are specialisations of various traits classes in the library and are mainly a collection of utility headers for the application developer

When running the tests in Debug mode on Windows, the assertion at line 285 of movetk/algo/Interpolation.h is triggered:
assert(eps * eps < MOVETK_EPS);

This happens during test case 'kinematic trajectory interpolation 1'. eps is a MovetkVector with coordinates (6.4709688360932933, 5.1473812288972436) for me, which evidently is much larger than MOVETK_EPS when "squared".

I'm not sure whether the assertion is incorrect or the code does something interesting. @aniketmitra001 do you know what causes this issue?

This way we can enforce some standard on code styling and formatting, and makes it easier for users to contribute. We should decide what kind of code style we want for this.

The current implementation of Statistics.h does not seem to be able to handle degenerate cases , for instance when the trajectory size is 1.
For input data where trajectory has only one point, this results in a segmentation fault.

I think its better to handle this..

In the development meeting of October 2nd,2020 the following was suggested:

• Discussed whether to integrate map matcher into MoveTK, consensus is to add tutorials and/or documentation on how to interface with the most common mapmatchers like grasshopper and Valhalla.
• Related to map matching, there is a need to provide a network/graph representation for some algorithms into MoveTK.
• The suggestion was to pick one algorithm as a first use case to guide development, as MoveTK would need to need a proper representation of a network (wherein the graph has a corresponding geometric representation as well). The question is, how can MoveTK have such a data structure?
  • It was suggested that MoveTK could follow a similar design pattern as the existing geometric backend. Provide a generic specification to the graph: in terms of operators/representation as a concept
• Discussion around which graph library to support as there is some existing support for boost graph, suggestion to look into other libraries such as igraph.

The implementation of algorithms in movetk/algo/Statistics.h assumes that the input trajectory is of type TabularTrajectory or ColumnarTrajectory.

However, sometimes the trajectory type may not necessarily be of these two types, for example the return type of Splitter::begin()

So maybe its better to use ``TrajectoryTraitsto infer the type of a particular field instead of usingtypename Trajectory::template FieldType```

make fails with the following error

[ 98%] Building CXX object py/CMakeFiles/movetk_geometry.dir/movetk_geometry.cpp.o
/home/runner/work/movetk/movetk/py/movetk_geometry.cpp:44:16: error: expected constructor, destructor, or type conversion before ‘(’ token
 PYBIND11_MODULE(movetk_geometry, m) {