Euclidean generator and tsp enhancements#467
Open
megyhazy wants to merge 25 commits intoboostorg:developfrom
Open
Euclidean generator and tsp enhancements#467megyhazy wants to merge 25 commits intoboostorg:developfrom
megyhazy wants to merge 25 commits intoboostorg:developfrom
Conversation
…raph generation functions
…thub.com/megyhazy/graph into euclidean_generator_and_tsp_enhancements
Collaborator
|
Hey, thank you, this looks great! I'm just wondering about how generic to make it now or later. I'm thinking along two separate lines:
The Boost.Geometry syntax is What do you think? I don't want to hold up your work for too long but I think this should actually be a fairly small change for a big flexibility. Edit: Removed my stupid comment before anyone noticed. :) |
Author
|
Jeremy, thank you for the suggestion. I will look at this over the
weekend.
My other enhancement idea is re: creating random complete vs incomplete
graphs. Although for TSP complete is typically the useful configuration, I
could see that for other purposes it could make sense to have parameterized
incomplete graphs also. Perhaps not for this submission but your feedback
would be helpful.
Sent from Gmail Mobile, please forgive typos.
…On Tue, Apr 14, 2026 at 5:33 AM Jeremy W. Murphy ***@***.***> wrote:
*jeremy-murphy* left a comment (boostorg/graph#467)
<#467 (comment)>
Hey, thank you, this looks great!
I'm just wondering about how generic to make it now or later. I'm thinking
along two separate lines:
- the difference between a euclidean and non-euclidean graph is
entirely in the distance calculation, meaning most of the code for
generating a random graph is the same
- Boost.Geometry is an obvious choice for representing geometry in the
first place, so I want their users to easily be able to provide data
without converting their types.
Although I think the natural expression for distance between two points a
and b is simply a - b, Boost.Geometry doesn't provide that syntax. 😅
Their syntax is distance(a, b). So I'm more inclined to make our code do
the same thing, such that a Boost.Geometry point type will just work, which
I guess means that we define distance(boost::simple_point,
boost::simple_point) to use std::hypot and use distance(a, b) in our
code. This actually solves both problems at once, because euclidean and
non-euclidean points in Boost.Geometry have the same interface. I guess it
means that these functions would no longer be specific to euclidean
geometry and so they would be better named make_geometric_graph, etc.
What do you think? I don't want to hold up your work for too long but I
think this should actually be a fairly small change for a big flexibility.
—
Reply to this email directly, view it on GitHub
<#467 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BTCHAOBTLN4OSE4NN7GDE2D4VYAVJAVCNFSM6AAAAACXMZ2LWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DENBSHA2DGMRTGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
…s euclidean - and compatible with boost.geometry
…s euclidean - and compatible with boost.geometry
…s euclidean - and compatible with boost.geometry
Author
|
Jeremy,
I made this adjustment and pushed . It was messier than expected which
leads me to believe that there must be a better way. Two items of note:
- Did not work if I tried to fully rely on ADL, it would not pick up the
boost::geometric::distance overloads. I had to bring them in with a
'using' statement at the top of the function connect_all_geometric.
- Accessing points in boost::geometry uses x() and y() which got ugly with
simple_point. I wrote some type-based specialization to accommodate but it
was awkward:
// Traits class for coordinate access abstractiontemplate < typename
Point, typename Enable = void >struct geometric_point_traits
{
static auto x(const Point& p) -> decltype(p.x) { return p.x; }
static auto y(const Point& p) -> decltype(p.y) { return p.y; }
};
/// Partial specialization for all Boost.Geometry point typestemplate
< typename Point >struct geometric_point_traits< Point,
typename std::enable_if<
std::is_same< typename boost::geometry::traits::tag< Point >::type,
boost::geometry::point_tag >::value >::type >
{
static auto x(const Point& p) -> decltype(boost::geometry::get< 0 >(p))
{
return boost::geometry::get< 0 >(p);
}
static auto y(const Point& p) -> decltype(boost::geometry::get< 1 >(p))
{
return boost::geometry::get< 1 >(p);
}
};
Perhaps there is a better way to do this?
On Wed, Apr 15, 2026 at 6:48 PM Matyas Egyhazy ***@***.***>
wrote:
Jeremy, thank you for the suggestion. I will look at this over the
weekend.
My other enhancement idea is re: creating random complete vs incomplete
graphs. Although for TSP complete is typically the useful configuration, I
could see that for other purposes it could make sense to have parameterized
incomplete graphs also. Perhaps not for this submission but your feedback
would be helpful.
Sent from Gmail Mobile, please forgive typos.
On Tue, Apr 14, 2026 at 5:33 AM Jeremy W. Murphy ***@***.***>
wrote:
> *jeremy-murphy* left a comment (boostorg/graph#467)
> <#467 (comment)>
>
> Hey, thank you, this looks great!
>
> I'm just wondering about how generic to make it now or later. I'm
> thinking along two separate lines:
>
> - the difference between a euclidean and non-euclidean graph is
> entirely in the distance calculation, meaning most of the code for
> generating a random graph is the same
> - Boost.Geometry is an obvious choice for representing geometry in
> the first place, so I want their users to easily be able to provide data
> without converting their types.
>
> Although I think the natural expression for distance between two points a
> and b is simply a - b, Boost.Geometry doesn't provide that syntax. 😅
> Their syntax is distance(a, b). So I'm more inclined to make our code do
> the same thing, such that a Boost.Geometry point type will just work, which
> I guess means that we define distance(boost::simple_point,
> boost::simple_point) to use std::hypot and use distance(a, b) in our
> code. This actually solves both problems at once, because euclidean and
> non-euclidean points in Boost.Geometry have the same interface. I guess it
> means that these functions would no longer be specific to euclidean
> geometry and so they would be better named make_geometric_graph, etc.
>
> What do you think? I don't want to hold up your work for too long but I
> think this should actually be a fairly small change for a big flexibility.
>
> —
> Reply to this email directly, view it on GitHub
> <#467 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BTCHAOBTLN4OSE4NN7GDE2D4VYAVJAVCNFSM6AAAAACXMZ2LWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DENBSHA2DGMRTGQ>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
Virus-free.www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
<#m_6853456666640036676_DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
|
…s euclidean - and compatible with boost.geometry
…s euclidean - and compatible with boost.geometry
…s euclidean - and compatible with boost.geometry
…s euclidean - and compatible with boost.geometry
Collaborator
|
Hmm ok, thanks for giving it a go. I'll take a closer look on Monday or Tuesday. |
jeremy-murphy
requested changes
Apr 18, 2026
| boost::generate_random_points( | ||
| num_points, 10000, std::back_inserter(points)); | ||
|
|
||
| std::set< std::pair< double, double > > unique_points; |
Collaborator
There was a problem hiding this comment.
Although performance doesn't matter here, it's a good habit to get into using boost::unordered_flat_(set|map) in preference to std::(set|map) or std::unordered_(set|map) when the requirements are equivalent (such as here).
| std::set< std::pair< double, double > > unique_points; | ||
| for (const auto& p : points) | ||
| { | ||
| unique_points.insert(std::make_pair(p.x, p.y)); |
Collaborator
There was a problem hiding this comment.
If you assert on the result of this insertion then the test will fail as soon as the first duplicate is hit rather than waiting until the end. (Admittedly, not a big deal when n = 100.) It might not be so useful for random graphs, but for non-random graphs it is then straightforward to print which value it failed on, which is more useful than only knowing that size() != num_points.
| #include <boost/unordered/unordered_flat_set.hpp> | ||
| #include <boost/concept/assert.hpp> | ||
| #include <boost/graph/graph_concepts.hpp> | ||
| #include <boost/geometry.hpp> |
Collaborator
There was a problem hiding this comment.
Ah, so just quickly, this file shouldn't refer to Boost Geometry at all, only an example file should. We're going to copy the Boost Geometry interface here without referring to it directly.
If that doesn't make sense, don't worry, I'll have a go at getting it to work next week.
Author
|
Thanks Jeremy. I tried that before but must have been doing something
wrong. The distance function was not resolving to the boost::geometry
overloads. Instead it was hijacked by a version with iterators. Looking
forward to learning a thing or two :-)
Will look at the other comments l.
Sent from Gmail Mobile, please forgive typos.
…On Sat, Apr 18, 2026 at 8:49 AM Jeremy W. Murphy ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In test/geometric_graph_generator_test.cpp
<#467?email_source=notifications&email_token=BTCHAOGF3EVI5JD6VCPDJID4WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K64DSL5ZGK5TJMV3V6Y3MNFRWW#discussion_r3078197086>
:
> +
+//=======================================================================
+// Test Suite: generate_random_points
+//=======================================================================
+
+BOOST_AUTO_TEST_SUITE(generate_random_points_tests)
+
+BOOST_AUTO_TEST_CASE(test_uniqueness)
+{
+ const std::size_t num_points = 100;
+ std::vector< PointDbl > points;
+
+ boost::generate_random_points(
+ num_points, 10000, std::back_inserter(points));
+
+ std::set< std::pair< double, double > > unique_points;
Although performance doesn't matter here, it's a good habit to get into
using boost::unordered_flat_(set|map) in preference to std::(set|map) or
std::unordered_(set|map) when the requirements are equivalent (such as
here).
------------------------------
In test/geometric_graph_generator_test.cpp
<#467?email_source=notifications&email_token=BTCHAOGF3EVI5JD6VCPDJID4WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K64DSL5ZGK5TJMV3V6Y3MNFRWW#discussion_r3078221438>
:
> +//=======================================================================
+
+BOOST_AUTO_TEST_SUITE(generate_random_points_tests)
+
+BOOST_AUTO_TEST_CASE(test_uniqueness)
+{
+ const std::size_t num_points = 100;
+ std::vector< PointDbl > points;
+
+ boost::generate_random_points(
+ num_points, 10000, std::back_inserter(points));
+
+ std::set< std::pair< double, double > > unique_points;
+ for (const auto& p : points)
+ {
+ unique_points.insert(std::make_pair(p.x, p.y));
If you assert on the result of this insertion then the test will fail as
soon as the first duplicate is hit rather than waiting until the end.
(Admittedly, not a big deal when n = 100.) It might not be so useful for
random graphs, but for non-random graphs it is then straightforward to
print *which* value it failed on, which is more useful than only knowing
that size() != num_points.
------------------------------
In include/boost/graph/geometric_graph_generator.hpp
<#467?email_source=notifications&email_token=BTCHAOGF3EVI5JD6VCPDJID4WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K64DSL5ZGK5TJMV3V6Y3MNFRWW#discussion_r3105103567>
:
> +// accompanying file LICENSE_1_0.txt or copy at
+// http://www.boost.org/LICENSE_1_0.txt)
+//=======================================================================
+
+#ifndef BOOST_GRAPH_GEOMETRIC_GRAPH_GENERATOR_HPP
+#define BOOST_GRAPH_GEOMETRIC_GRAPH_GENERATOR_HPP
+
+#include <boost/graph/graph_traits.hpp>
+#include <boost/graph/properties.hpp>
+#include <boost/graph/simple_point.hpp>
+#include <boost/static_assert.hpp>
+#include <boost/container_hash/hash.hpp>
+#include <boost/unordered/unordered_flat_set.hpp>
+#include <boost/concept/assert.hpp>
+#include <boost/graph/graph_concepts.hpp>
+#include <boost/geometry.hpp>
Ah, so just quickly, this file shouldn't refer to Boost Geometry at all,
only an example file should. We're going to copy the Boost Geometry
interface here without referring to it directly.
If that doesn't make sense, don't worry, I'll have a go at getting it to
work next week.
—
Reply to this email directly, view it on GitHub
<#467?email_source=notifications&email_token=BTCHAOBXIYJDL65IX2GLMZ34WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2L24DSL5ZGK5TJMV3V63TPORUWM2LDMF2GS33OONPWG3DJMNVQ#pullrequestreview-4104544247>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BTCHAOBCQJI46IRI3MRWSJD4WN2UDAVCNFSM6AAAAACXMZ2LWCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DCMBUGU2DIMRUG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Author
|
Btw I could also make it work by passing the distance function as a
parameter but I don’t think that is what we were aiming for either. Anyway
- very excited to see your thoughts!
Sent from Gmail Mobile, please forgive typos.
On Sat, Apr 18, 2026 at 9:12 AM Matyas Egyhazy ***@***.***>
wrote:
… Thanks Jeremy. I tried that before but must have been doing something
wrong. The distance function was not resolving to the boost::geometry
overloads. Instead it was hijacked by a version with iterators. Looking
forward to learning a thing or two :-)
Will look at the other comments l.
Sent from Gmail Mobile, please forgive typos.
On Sat, Apr 18, 2026 at 8:49 AM Jeremy W. Murphy ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
> ------------------------------
>
> In test/geometric_graph_generator_test.cpp
> <#467?email_source=notifications&email_token=BTCHAOGF3EVI5JD6VCPDJID4WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K64DSL5ZGK5TJMV3V6Y3MNFRWW#discussion_r3078197086>
> :
>
> > +
> +//=======================================================================
> +// Test Suite: generate_random_points
> +//=======================================================================
> +
> +BOOST_AUTO_TEST_SUITE(generate_random_points_tests)
> +
> +BOOST_AUTO_TEST_CASE(test_uniqueness)
> +{
> + const std::size_t num_points = 100;
> + std::vector< PointDbl > points;
> +
> + boost::generate_random_points(
> + num_points, 10000, std::back_inserter(points));
> +
> + std::set< std::pair< double, double > > unique_points;
>
> Although performance doesn't matter here, it's a good habit to get into
> using boost::unordered_flat_(set|map) in preference to std::(set|map) or
> std::unordered_(set|map) when the requirements are equivalent (such as
> here).
> ------------------------------
>
> In test/geometric_graph_generator_test.cpp
> <#467?email_source=notifications&email_token=BTCHAOGF3EVI5JD6VCPDJID4WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K64DSL5ZGK5TJMV3V6Y3MNFRWW#discussion_r3078221438>
> :
>
> > +//=======================================================================
> +
> +BOOST_AUTO_TEST_SUITE(generate_random_points_tests)
> +
> +BOOST_AUTO_TEST_CASE(test_uniqueness)
> +{
> + const std::size_t num_points = 100;
> + std::vector< PointDbl > points;
> +
> + boost::generate_random_points(
> + num_points, 10000, std::back_inserter(points));
> +
> + std::set< std::pair< double, double > > unique_points;
> + for (const auto& p : points)
> + {
> + unique_points.insert(std::make_pair(p.x, p.y));
>
> If you assert on the result of this insertion then the test will fail as
> soon as the first duplicate is hit rather than waiting until the end.
> (Admittedly, not a big deal when n = 100.) It might not be so useful for
> random graphs, but for non-random graphs it is then straightforward to
> print *which* value it failed on, which is more useful than only knowing
> that size() != num_points.
> ------------------------------
>
> In include/boost/graph/geometric_graph_generator.hpp
> <#467?email_source=notifications&email_token=BTCHAOGF3EVI5JD6VCPDJID4WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K64DSL5ZGK5TJMV3V6Y3MNFRWW#discussion_r3105103567>
> :
>
> > +// accompanying file LICENSE_1_0.txt or copy at
> +// http://www.boost.org/LICENSE_1_0.txt)
> +//=======================================================================
> +
> +#ifndef BOOST_GRAPH_GEOMETRIC_GRAPH_GENERATOR_HPP
> +#define BOOST_GRAPH_GEOMETRIC_GRAPH_GENERATOR_HPP
> +
> +#include <boost/graph/graph_traits.hpp>
> +#include <boost/graph/properties.hpp>
> +#include <boost/graph/simple_point.hpp>
> +#include <boost/static_assert.hpp>
> +#include <boost/container_hash/hash.hpp>
> +#include <boost/unordered/unordered_flat_set.hpp>
> +#include <boost/concept/assert.hpp>
> +#include <boost/graph/graph_concepts.hpp>
> +#include <boost/geometry.hpp>
>
> Ah, so just quickly, this file shouldn't refer to Boost Geometry at all,
> only an example file should. We're going to copy the Boost Geometry
> interface here without referring to it directly.
> If that doesn't make sense, don't worry, I'll have a go at getting it to
> work next week.
>
> —
> Reply to this email directly, view it on GitHub
> <#467?email_source=notifications&email_token=BTCHAOBXIYJDL65IX2GLMZ34WN2UDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMJQGQ2TINBSGQ32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2L24DSL5ZGK5TJMV3V63TPORUWM2LDMF2GS33OONPWG3DJMNVQ#pullrequestreview-4104544247>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BTCHAOBCQJI46IRI3MRWSJD4WN2UDAVCNFSM6AAAAACXMZ2LWCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DCMBUGU2DIMRUG4>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
Collaborator
|
Ahh, yeah, |
Author
|
Thanks - will do - probably next weekend.
Another item is if you have any advice for bridging the gap between
simple_point public access to x and y vs boost::geometry usage of x() and
y(). The solution I currently have in-place makes the algorithm
boost::geometry aware which I don't like.
One idea I have is to register simple_point as a boost::geometry point -
but I am not quite sure I understand the possible side effects of this
since it is a macro...would like your advice:
// Register simple_point<T> as a 2D cartesian point
BOOST_GEOMETRY_REGISTER_POINT_2D_TEMPLATE(
boost::simple_point, // point type template
T, // coordinate type template parameter
cs::cartesian, // coordinate system
x, y // member names
)
If it was c++ 20 I could do:
template<typename Point>
concept Point2D = requires(const Point& p) {
{ p.x } -> std::convertible_to<double>;
{ p.y } -> std::convertible_to<double>;
} || requires(const Point& p) {
{ p.x() } -> std::convertible_to<double>;
{ p.y() } -> std::convertible_to<double>;
};
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
Virus-free.www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail>
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
…On Sun, Apr 19, 2026 at 9:59 PM Jeremy W. Murphy ***@***.***> wrote:
*jeremy-murphy* left a comment (boostorg/graph#467)
<#467?email_source=notifications&email_token=BTCHAOABSXUKPTRNBAMZNID4WV77NA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRXG42DAOBSGE4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2LK4DSL5RW63LNMVXHIX3POBSW4X3DNRUWG2Y#issuecomment-4277408218>
Ahh, yeah, std::distance exists and will cause problems if it is found
via ADL. We might have to offer two overloads: one that uses distance
found via ADL and one that allows the user to pass the distance function in.
—
Reply to this email directly, view it on GitHub
<#467?email_source=notifications&email_token=BTCHAOABSXUKPTRNBAMZNID4WV77NA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMRXG42DAOBSGE4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2LK4DSL5RW63LNMVXHIX3POBSW4X3DNRUWG2Y#issuecomment-4277408218>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BTCHAOAQMAXGOWTQBVQ64FD4WV77NAVCNFSM6AAAAACXMZ2LWCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DENZXGQYDQMRRHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Collaborator
|
Using |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thoughts:
Addressed earlier comments: