1342 make geographic location available to all of MEmilio #1346
Conversation
kilianvolmer
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1346 +/- ##
========================================
Coverage 97.26% 97.27%
========================================
Files 176 179 +3
Lines 15439 15545 +106
========================================
+ Hits 15017 15121 +104
- Misses 422 424 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Carlotta Gerstein <100771374+charlie0614@users.noreply.github.com> Co-authored-by: Martin J. Kühn <62713180+mknaranja@users.noreply.github.com>
cpp/tests/test_geography.cpp
Outdated
| /** | ||
| * @brief Test the r-Tree Constructor given a vector | ||
| */ | ||
| TEST_F(TestGeography, rtreeConstructionVector) |
There was a problem hiding this comment.
I think you should rather test that the constructor does what it is supposed to do rather than just test that it runs without an error.
Co-authored-by: HenrZu <69154294+HenrZu@users.noreply.github.com>
Co-authored-by: HenrZu <69154294+HenrZu@users.noreply.github.com>
cpp/memilio/geography/geolocation.h
Outdated
| void check_input() const | ||
| { | ||
| assert(m_latitude <= 90. && m_latitude >= -90. && "Latitude must be in [-90, 90]"); | ||
| assert(m_longitude <= 180. && m_longitude >= -180. && "Longitude must be in [-180, 180]"); | ||
| } |
There was a problem hiding this comment.
I think this should be private. Also please add a short @brief that this (only) calls asserts.
There was a problem hiding this comment.
A possible alternative to using asserts, would be to add a "parse" member function
static IOResult<GeographicalLocation> parse(ScalarType, ScalarType);
that constructs a location for valid coordinates and returns an error otherwise. This would make sense if we use this for user input a lot.
cpp/memilio/geography/rtree.h
Outdated
| * @param radius The radius of the query in kilometers. | ||
| * @return Vector with indices of the points found. | ||
| */ | ||
| std::vector<size_t> inrange_indices_approximate(const IsSphericalLocation auto& location, double radius) const |
There was a problem hiding this comment.
I am slightly confused by this "inrange", is this an established phrase? Is the name indices_in_radius_approximate better?
There was a problem hiding this comment.
I think that inrange is an established phrase. At least I can show you other packages using it.
There was a problem hiding this comment.
Hm, but it seems like it is still meant as "in range", and not a single word, right? So it should be "in_range" in our naming convention.
I still slightly prefer my suggenstion "indices_in_radius_approximate": I feel like "indices_in_range_approximate" is a more intuitive order. And we ask for a radius, rather than a range (in the sense of std::ranges).
There was a problem hiding this comment.
To me my naming feels more natural. Especially I would start with the inrange as this is the first thing you think of when searching for this function.
There was a problem hiding this comment.
as this is the first thing you think of when searching for this function
No, I would not think of that. And typing "inrange" in most IDEs should bring up "indices_in_range" as well.
Anyways, we should not break our naming conventions for this, so please use "in_range".
cpp/memilio/geography/rtree.h
Outdated
| template <IsSphericalLocationIterator Iter> | ||
| RTree(Iter first, Iter last) | ||
| : rtree{} | ||
| { | ||
| size_t index = 0; | ||
| while (first != last) { | ||
| Point point(first->get_longitude(), first->get_latitude()); | ||
| rtree.insert(Node(point, index)); | ||
| ++first; | ||
| ++index; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you need this constructor, when the vector version already exists?
A while loop over user provided iterators is a common source of segfaults or infinite loops, so I would prefer to remove this. In most cases, the vector constructor can be used equivalently via RTree({begin(), end()}).
There was a problem hiding this comment.
Doesn't this give more flexibility for data not stored in vectors?
There was a problem hiding this comment.
Not really, RTree({begin(), end()}) can be used with any iterable. It creates a vector from two iterators, then uses the construction from vector. At worst, you have an additional copy of the elements.
You could change the vector constructor, so that it takes a std::ranges::range instead, and make use of a range based for loop. But I honestly think the vector is enough.
Co-authored-by: reneSchm <49305466+reneSchm@users.noreply.github.com>
…o' of github.com:SciCompMod/memilio into 1342-Make-GeographicLocation-available-to-all-of-MEmilio
5 participants
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
GeographicLocationtomemilio/geography/locations.hGeographicLocations on the sphereGeographicLocationIf need be, add additional information and what the reviewer should look out for in particular:
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)
Closes #1342