[gdal-dev] RFC 48: Geographical networks support

[Mikhail Gusev]

Even, firstly thanks for your remarks!

Not much to say on the RFC itself. Perhaps you should mention who will take
> care of committing in the source : Dmitry I guess ?
>

Yes, I think someone who has committing rights can do this. I will discuss
it with Dmitriy.


> I looked at the code changes:
> https://github.com/MikhanGusev/gdal/compare/OSGeo:trunk...trunk
>
> My remarks:
> - any reason not to licence  autotest/gnm/gnm_gdalnetwork_test.py
>  under X/MIT ? There are a few files LGPL licenced in autotest, but this is
> more historical remain than intended policy.
>

No problems with X/MIT.

-  gdal/apps/GNUmakefile : in clean target, analysis/*.o is probably
> unneeded
>
- which gnm headers are installed ? gnm.h gnm_frmts.h gnm_api.h
> gnmstdanalysis.h ?
>

Actually exactly these 4 files must be installed. I'll add them to
makefiles.

- header files should have guards against multiple inclusion. At least seen
> for
> gnm/analysis/gnmstdanalysis.h
>

My mistake, I'll fix this.


> - in implementations there are many maps, vectors, etc., so I guess some
> algorithms may consume a lot of memory if operating on huge networks. The
> documentation should make it clear.
>

The use of these containers make the calculations fast. And what I haven't
done actually - is the testing on the very huge graphs. I'll include this
to my future plans. No problems with docs.


> - GNMGdalStdRoutingAnalyser and GNMGdalStdCommutationsAnalyser: currently
> have
> only one method and no member variables. Do you foresee future extensions
> to
> add member variables ? If not, they might go to GNMGdalStdAnalyser, no ?
> And
> what about non-GDAL network ? Shouldn't the interface of
> DijkstraShortestPath() and ResourceDestribution() go to GNMStdAnalyser ?
> Open
> questions : just wondering of what it might look like if other network
> implementations are done. I can imagine that the dijkstra alg would have
> similar interface.
>

According to my idea GNMStdAnalyser is actually the class which firstly
represents a graph and the methods to operate it. So it must contain only
the basic algorithms. DijkstraShortestPath() and ResourceDestribution() are
the methods which use the basic methods of the graph and return the results
in the specific form. Also in general for the classes
GNMGdalStdRoutingAnalyser and GNMGdalStdCommutationsAnalyser: they were
designed as the "specific-purpose" classes approximately for engineering
and road networks, so they can be extended in future and probably with
member variables. But this relates to the answer for the next remark:

- gnm/frmts/gnm_frmts.h :  I'm a bit concerned about exposing (installed
> header + CPL_DLL) an interface that has not yet been implemented. My
> intuition
> is that it might change once the first one or two implementations have been
> made. So maybe keep it internal/experimental for now.
>

I agree that the inclusion of the interfaces/capabilities that can be (not
will be) extended in future is not a 100% good idea. I hoped that someone
will be interested or even I will have time to implement and extend
something of what I wrote at the "Future ideas" section in my RFC. But we
do not know exactly will it be implemented or not. So: (1) We can remove
for now all these interfaces "for future", which means to leave only
GNMGdalNetwork and one analysing class. (2) Try to implement these
capabilities: pgRouting for gnm_frmts.h and for example
GNMGdalStdRoutingAnalyser with some algorithm extension (K shortest path).


> - gnm/frmts/gnm_frmts.h : "static const char *GNMAllFormats[] = {" should
> not
> be exposed in a .h file
>
- gnm/frmts/gnm_frmts.h :  static std::vector<std::string> _regNetFrmts :
> same
> remark as above
>
- gnm/frmts/gnm_frmts.h :  GetFormatByName() and RegisterAllNetworkFormats()
> should not be in a .h and lack "namespace"
>
- gnm/gnm.h : static const char *GNMGdalSupportedDrivers[] should be moved
> somewhere else
>

Actually I have no idea where to place the gnm_frmts.h static fields. They
must be common for all formats. But if we decide to remove formats support
it will be easier.


> - gnm/gnm.h : most #define are not of public use. might be moved in a
> gnm_priv.h
>

Yes, the same for rule types.

- gnm/gnm.h : typedef int GNMDirection and the 3 following defines. A
> typedef
> enum would seem more appropriate. Same for GNMErr (although I can see this
> pattern in ogr_core.h. not clear why enum have not been chosen)
>

Agree.


> - gnm/gnm.h : GNMManager::Close() the GNMNetwork should know if it is
> native
> or not. The user shouldn't have to specify it again.
>
- gnm/gnm.h : GNMManager::GdalCloseNetwork(). Ideally this method should not
> exist and Close() should work for all kind of network objects.
>

Can be possible.


> - gnm/gnm.h : GNMNetwork class definition. Commented methods that could be
> removed. TODO that no longer apply
>
- gnm/gnm.h : IsDriverSupported() --> IsDatasetDriverSupported() for
> consistency between name and argument ?
>
- gnm/gnm.h : std::pair<char**,int> GetClassLayers () : very unusual
> interface
> in GDAL. I'd say return a char** NULL terminated for consistency to other
> methods in the same class
>

Agree.

- gnm/gnm_api.h: all functions are prefixed by GNMGdal. But some methods
> actually belong to the base GNMNetwork class, ant not specifically to
> GNMGdalNetwork. For example CreateLayer(), CreateFeature(), and many others
>

Yes, I should update the C header and also wrap some methods from
GNMManager.


> - gnm/gnm_tut.dox : #include "analysis/gnmstdanalysis.h" . Is the analysis
> directory valid for an installed GDAL ?
>

Haven't check.


> - gnm/gnmgdalnetwork.cpp : _makeNewLayerName(): newName might overflow.
> instead
> of 100, use strlen(name) + 50. Or return a std::string to avoid manual
> dealing
> with memory
>
- gnm/gnmgdalnetwork.cpp : GNMGdalNetwork::CreateLayer() : the call to
> _makeNewLayerName return a char* not a const char*.
>
- GNMGdalNetwork::IsDriverSupported (): CSLFindString() >= 0 could be used
> to
> replace the loop
>
- GNMGdalNetwork::GetSupportedDrivers () : return
> CSLDuplicate(GNMGdalSupportedDrivers)
>
- GNMGdalNetwork::GetMetaParamValueName () : return a "const char*" that
> must
> be freed by delete[]. Quite unusual. Better return a "char*". Other issue,
> it
> is allocated with new char[], but the method is bound to C. C callers
> cannot
> call delete[]. So you have to allocate with CPLMalloc(), and ask the
> caller to
> free with CPLFree(). Isn't there a risk of overflow with 254 characters ?
>
- GNMGdalNetwork::GetMetaParamValueDescr () : same remarks as above
>

I'll fix.

- GNMManager::CreateConnectivity () : I'm confused by the 'native' term. In
> this method, native=false seems to imply the GDAL "proprietary" network
> format
> that can work with any vector driver that has similar capabilities than
> shapefile. Whereas in the RFC text, it seems to imply the contrary (
> "network
> of ”GDAL-native” format").
>

Maybe I used it not correctly everywhere, but the general idea is the
following: The term "native" corresponds to the existed network formats (so
when we work with pgRouting network we work with its native tables and
fields, rather than with GNMGdal- system layers), while the GDAL-networks
are not "native" and more likely "common".

-  GNMNetwork::AutoConnect() :" if (lineGFID == 684) { int x = 9; } " debug
> left-over ?
>

Yes, my mistake :D
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/gdal-dev/attachments/20140909/a8d8c127/attachment.html>