Andoyer geographic distance

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Andoyer geographic distance

branimir
Hi,

I am trying to work with boost 1.57 and also I've got the geometry.extensions.gis from the develop branch. I am using geographic cs. Looking at extensions\gis\geographic\strategies\andoyer.hpp I think there is a problem with one of the constructors:

    explicit inline andoyer(RadiusType f)
        : m_ellipsoid(f)
    {}

I think should be:

    explicit inline andoyer(RadiusType f)
        : m_ellipsoid(f, f)
    {}

(note the second radius passed to m_ellipsoid). Otherwise it chooses the incorrect constructor for the ellipsoid and distances are incorrect.

Sorry for posting it in the groups - I am not sure what the proper way to submit a patch is.

Best,
Branimir

_______________________________________________
Geometry mailing list
[hidden email]
http://lists.boost.org/mailman/listinfo.cgi/geometry
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Andoyer geographic distance

Adam Wulkiewicz
Hi,

2014-11-11 1:53 GMT+01:00 Branimir Betov <[hidden email]>:
Hi,

I am trying to work with boost 1.57 and also I've got the geometry.extensions.gis from the develop branch. I am using geographic cs. Looking at extensions\gis\geographic\strategies\andoyer.hpp I think there is a problem with one of the constructors:

    explicit inline andoyer(RadiusType f)
        : m_ellipsoid(f)
    {}

I think should be:

    explicit inline andoyer(RadiusType f)
        : m_ellipsoid(f, f)
    {}

(note the second radius passed to m_ellipsoid). Otherwise it chooses the incorrect constructor for the ellipsoid and distances are incorrect.


I'm not entirely sure what's the purpose of this ctor and one-argument ctor of ellipsoid. I'd say that the intention was to create a "unit" ellipsoid with radius=1 and flattening f, but there is an error in the ellipsoid ctor (it doesn't set B, only A and F). There is a reference to a unit sphere (not ellipsoid) in the comment:

// Unit sphere
ellipsoid(T const& f)
: m_a(1.0)
, m_f(f)
{}

https://github.com/boostorg/geometry/blob/develop/include/boost/geometry/extensions/gis/geographic/detail/ellipsoid.hpp

I plan to clean this code and move from extensions in the near future. I think that this ctor could just dissapear. In the meanwhile I may just fix it by setting B properly.
 
Sorry for posting it in the groups - I am not sure what the proper way to submit a patch is.


This group is a right place for such discussion :)

A patch could be sent to this group or proposed directly on GitHub. I personally prefer the latter. Here is a tutorial: https://github.com/boostorg/geometry/wiki/Contribution-Tutorial

Regards,
Adam

_______________________________________________
Geometry mailing list
[hidden email]
http://lists.boost.org/mailman/listinfo.cgi/geometry
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Andoyer geographic distance

Barend
Hi,

Adam Wulkiewicz wrote On 12-11-2014 0:49:
Hi,

2014-11-11 1:53 GMT+01:00 Branimir Betov <[hidden email]>:
Hi,

I am trying to work with boost 1.57 and also I've got the geometry.extensions.gis from the develop branch. I am using geographic cs. Looking at extensions\gis\geographic\strategies\andoyer.hpp I think there is a problem with one of the constructors:

    explicit inline andoyer(RadiusType f)
        : m_ellipsoid(f)
    {}

I think should be:

    explicit inline andoyer(RadiusType f)
        : m_ellipsoid(f, f)
    {}

(note the second radius passed to m_ellipsoid). Otherwise it chooses the incorrect constructor for the ellipsoid and distances are incorrect.


I'm not entirely sure what's the purpose of this ctor and one-argument ctor of ellipsoid. I'd say that the intention was to create a "unit" ellipsoid with radius=1 and flattening f, but there is an error in the ellipsoid ctor (it doesn't set B, only A and F). There is a reference to a unit sphere (not ellipsoid) in the comment:

// Unit sphere
ellipsoid(T const& f)
: m_a(1.0)
, m_f(f)
{}


Yes, I think this should go. It is (most probably) never used.


I plan to clean this code and move from extensions in the near future. I think that this ctor could just dissapear. In the meanwhile I may just fix it by setting B properly.


Agreed, that ctor should disappear. I think it has been used (by me) to compare between Haversine and this, and ended up in the repository.

Please remove it, or I can do it.

 
Sorry for posting it in the groups - I am not sure what the proper way to submit a patch is.


This group is a right place for such discussion :)

A patch could be sent to this group or proposed directly on GitHub. I personally prefer the latter. Here is a tutorial: https://github.com/boostorg/geometry/wiki/Contribution-Tutorial


Thanks for reporting this.

Regards, Barend



_______________________________________________
Geometry mailing list
[hidden email]
http://lists.boost.org/mailman/listinfo.cgi/geometry
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Andoyer geographic distance

Adam Wulkiewicz
Hi,

Barend Gehrels wrote:
Adam Wulkiewicz wrote On 12-11-2014 0:49:

I plan to clean this code and move from extensions in the near future. I think that this ctor could just dissapear. In the meanwhile I may just fix it by setting B properly.


Agreed, that ctor should disappear. I think it has been used (by me) to compare between Haversine and this, and ended up in the repository.

Please remove it, or I can do it.

Ok done.

Regards,
Adam

_______________________________________________
Geometry mailing list
[hidden email]
http://lists.boost.org/mailman/listinfo.cgi/geometry
Loading...