Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Markus Neteler]

On Wed, Oct 14, 2015 at 5:16 AM, Vaclav Petras  wrote:
> la.h removed from gmath.h in r66485. I tested that with and without LAPACK
> configured. Work as expected. Perhaps the #error can be removed from the
> modules if it makes sense to have it in la.h (as I did that).

I suppose yes, but only if gmath.h gets also updated in relbranch70 to
avoid user confusion with addons.

Markus
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Vaclav Petras]

On Mon, Oct 12, 2015 at 6:53 AM, Glynn Clements 
wrote:
>
> > Having la.h as a separate header would conveniently hide the compilation
> > issue on that Debian server.
> >
> > So, should I remove la.h from gmath.h?
>
> I think so.

la.h removed from gmath.h in r66485. I tested that with and without LAPACK
configured. Work as expected. Perhaps the #error can be removed from the
modules if it makes sense to have it in la.h (as I did that).

https://trac.osgeo.org/grass/changeset/66485
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Glynn Clements]

Vaclav Petras wrote:

> I like the idea of a separate header but I would leave the implementation
> in lib/gmath. Or is this going too much against how other libraries are
> done? I'm also not sure how the prefixes should work, but these seems OK
> since there are functions with same prefix (G_) in several libraries.

Having separate headers for self-contained sections of a library isn't
an issue.

Ideally, if a localised section of a library has dependencies not
required for the rest of the library, that section is a candidate for
being a separate library (so that a lack of the dependency only
disables the features which require it, rather than the entire
library).

But this isn't rigorously followed (e.g. libgis has compile-time
dependencies on iconv, pthread, regex, and zlib, even though each of
those libraries is only used for isolated features).

> Having la.h as a separate header would conveniently hide the compilation
> issue on that Debian server.
> 
> So, should I remove la.h from gmath.h?

I think so.

-- 
Glynn Clements 
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Vaclav Petras]

On Wed, Oct 7, 2015 at 7:00 PM, Glynn Clements 
wrote:
>
> Vaclav Petras wrote:
>
> > I was not able to determine from svn why gmath.h contains la.h.
>
> AFAICT, it's because the functions are in lib/gmath. Of course, that
> then brings up the question why they're there, rather than in a
> separate library ...
>
> Incidentally, the whole of lib/gmath/la.c is conditionalised upon
>
> #if defined(HAVE_LIBLAPACK) && defined(HAVE_LIBBLAS)
>
> but individual functions (and even sections of functions) are
> conditionalised as necessary.

I have seen this code before and I was not courageous enough to clean it up.

> And there are a few functions for which we could reasonably supply our
> own implementation if BLAS/LAPACK aren't available. E.g.
> G_matrix_product() and G_vector_norm_euclid() should be
> straightforward.

Perhaps this is the idea of having the functionality in gmath itself is
behind having it part of gmath. Then the question is if we actually want
this or we want to move it. If we will be able to provide our
implementations, it doesn't matter where the functions are.

I like the idea of a separate header but I would leave the implementation
in lib/gmath. Or is this going too much against how other libraries are
done? I'm also not sure how the prefixes should work, but these seems OK
since there are functions with same prefix (G_) in several libraries.

Having la.h as a separate header would conveniently hide the compilation
issue on that Debian server.

So, should I remove la.h from gmath.h?

> AFAICT, G_matrix_LU_solve() is the only function that's moderately
> complex.
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Glynn Clements]

Vaclav Petras wrote:

> I was not able to determine from svn why gmath.h contains la.h.

AFAICT, it's because the functions are in lib/gmath. Of course, that
then brings up the question why they're there, rather than in a
separate library ...

Incidentally, the whole of lib/gmath/la.c is conditionalised upon

#if defined(HAVE_LIBLAPACK) && defined(HAVE_LIBBLAS)

but individual functions (and even sections of functions) are
conditionalised as necessary.

And there are a few functions for which we could reasonably supply our
own implementation if BLAS/LAPACK aren't available. E.g. 
G_matrix_product() and G_vector_norm_euclid() should be
straightforward.

AFAICT, G_matrix_LU_solve() is the only function that's moderately
complex.

-- 
Glynn Clements 
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Markus Neteler]

On Tue, Oct 6, 2015 at 10:03 AM, Moritz Lennert
 wrote:
> On 05/10/15 16:49, Vaclav Petras wrote:
...
>> Sorry for being -pedantic but I don't think we should leave the code
>> messy.
>
> +1

Then it needs to be removed everywhere here:

cd grass-addons
find . -type f | grep -v pristin | xargs grep 'la.h>'
./grass6/imagery/i.spec.unmix/global.h:#include 
./grass6/imagery/i.spec.unmix/la_extra.c:#include 
./grass6/imagery/i.spec.grass63/include/gmath.h:#include 
./grass6/imagery/i.spec.grass63/lib/gmath/la.c:#include 
./grass7/imagery/i.spec.unmix/global.h:#include 
./grass7/vector/v.kriging/local_proto.h:#include 


Markus
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Moritz Lennert]

On 05/10/15 16:49, Vaclav Petras wrote:

On Mon, Oct 5, 2015 at 3:44 AM, Martin Landa > wrote:
 >
 > Hi,
 >
 > 2015-10-05 9:41 GMT+02:00 Martin Landa >:
 > > compilation was simply failing on build server (Debian stable):

It is working on Ubuntu and probably Fedora as well. Perhaps it is a
local issue.

 > >
 > >> In file included from main.c:47:0:
 > >> global.h:28:29: error: unknown type name ‘vec_struct’
 > >> global.h:28:43: error: unknown type name ‘vec_struct’
 > >> global.h:31:8: error: unknown type name ‘mat_struct’
 > > ...
 >
 > even GRASS is compiled with lapack and blas support. I checked
config.h again:
 >
 > /* define if LAPACK exists */
 > #define HAVE_LIBLAPACK 1
 >
 > /* define if BLAS exists */
 > #define HAVE_LIBBLAS 1

Then la.h should be included in gmath.h. Including it in modules in a
workaround, not the solution. There are three possible solutions:

1) remove la.h from gmath.h and declare that modules must include it (I
don't see a reason for this)

2) change la.h include in gmath.h to something else; perhaps #if
defined(...) && defined(...) is wrong (but I don't think so, seems to
fit with the standard)

3) find out why it is different on that server and think about the
proper fix afterwards (ideal)

I can't test it since I can't reproduce it.

Sorry for being -pedantic but I don't think we should leave the code messy.


+1
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Vaclav Petras]

On Tue, Oct 6, 2015 at 4:22 AM, Markus Neteler  wrote:
>
> On Tue, Oct 6, 2015 at 10:03 AM, Moritz Lennert
>  wrote:
> > On 05/10/15 16:49, Vaclav Petras wrote:
> ...
> >> Sorry for being -pedantic but I don't think we should leave the code
> >> messy.
> >
> > +1
>
> Then it needs to be removed everywhere here:

What we need however, is to figure out why it is not working on that Debian
server. make distclean? svn revert? Dockerfile for Debian would be helpful
in this case to have reproducible environment.

> cd grass-addons
> find . -type f | grep -v pristin | xargs grep 'la.h>'
> ./grass6/imagery/i.spec.unmix/global.h:#include 
> ./grass6/imagery/i.spec.unmix/la_extra.c:#include 
> ./grass6/imagery/i.spec.grass63/include/gmath.h:#include 
> ./grass6/imagery/i.spec.grass63/lib/gmath/la.c:#include 
> ./grass7/imagery/i.spec.unmix/global.h:#include 
> ./grass7/vector/v.kriging/local_proto.h:#include 

I would remove it only in 7. It should work and it worked without it
(except for that Debian server). I don't know what is the situation in 6.
Let's not be bothered with code correctness for 6, the only priority there
is that the main code works and addons are ported to 7.
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Vaclav Petras]

On Tue, Oct 6, 2015 at 12:26 PM, Glynn Clements 
wrote:

>
> Markus Neteler wrote:
>
> > >> Sorry for being -pedantic but I don't think we should leave the code
> > >> messy.
> > >
> > > +1
> >
> > Then it needs to be removed everywhere here:
> >
> > cd grass-addons
> > find . -type f | grep -v pristin | xargs grep 'la.h>'
> > ./grass6/imagery/i.spec.unmix/global.h:#include 
> > ./grass6/imagery/i.spec.unmix/la_extra.c:#include 
> > ./grass6/imagery/i.spec.grass63/include/gmath.h:#include 
> > ./grass6/imagery/i.spec.grass63/lib/gmath/la.c:#include 
> > ./grass7/imagery/i.spec.unmix/global.h:#include 
> > ./grass7/vector/v.kriging/local_proto.h:#include 
>
> Sorry for joining in late, but from my understanding of the code, it
> seems that the inclusion of  should be in the modules, not
> in .
>
>  defines a number of macros plus the G_math_spvector
> type. It then includes  which contains a number of
> function declarations, some of which use G_math_spvector in their
> return or parameter types.
>
>  also includes , but doesn't depend upon
> anything defined in that header. If the include of  is
> removed, compiling a source file containing only
>
> #include 
>
> succeeds. Consequently, there is no need for  to
> include .
>
> Having one header for "base" functionality which is always available
> and another for additional functionality which is only available when
> GRASS was built with BLAS/LAPACK support seems cleaner than having a
> single header expose both, particularly when the two have already been
> separated.
>
> Additionally, requiring individual modules to include 
> explicitly would make it clear when code depends upon BLAS/LAPACK.
>

I agree.

I was not able to determine from svn why gmath.h contains la.h.


> --
> Glynn Clements 
> ___
> grass-dev mailing list
> grass-dev@lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/grass-dev
>
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Glynn Clements]

Markus Neteler wrote:

> >> Sorry for being -pedantic but I don't think we should leave the code
> >> messy.
> >
> > +1
> 
> Then it needs to be removed everywhere here:
> 
> cd grass-addons
> find . -type f | grep -v pristin | xargs grep 'la.h>'
> ./grass6/imagery/i.spec.unmix/global.h:#include 
> ./grass6/imagery/i.spec.unmix/la_extra.c:#include 
> ./grass6/imagery/i.spec.grass63/include/gmath.h:#include 
> ./grass6/imagery/i.spec.grass63/lib/gmath/la.c:#include 
> ./grass7/imagery/i.spec.unmix/global.h:#include 
> ./grass7/vector/v.kriging/local_proto.h:#include 

Sorry for joining in late, but from my understanding of the code, it
seems that the inclusion of  should be in the modules, not
in .

 defines a number of macros plus the G_math_spvector
type. It then includes  which contains a number of
function declarations, some of which use G_math_spvector in their
return or parameter types.

 also includes , but doesn't depend upon
anything defined in that header. If the include of  is
removed, compiling a source file containing only

#include 

succeeds. Consequently, there is no need for  to
include .

Having one header for "base" functionality which is always available
and another for additional functionality which is only available when
GRASS was built with BLAS/LAPACK support seems cleaner than having a
single header expose both, particularly when the two have already been
separated.

Additionally, requiring individual modules to include 
explicitly would make it clear when code depends upon BLAS/LAPACK.

-- 
Glynn Clements 
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Martin Landa]

Hi,

2015-10-05 3:04 GMT+02:00 Vaclav Petras :
> this shouldn't be necessary and same for r66404, gmath.h contains already
> grass/la.h. Any possible compilation issue is an issue with configuration
> (local or in GRASS) or with the gmath.h file. What was the reason for the
> change?

compilation was simply failing on build server (Debian stable):

> In file included from main.c:47:0:
> global.h:28:29: error: unknown type name ‘vec_struct’
> global.h:28:43: error: unknown type name ‘vec_struct’
> global.h:31:8: error: unknown type name ‘mat_struct’
...

Ma

-- 
Martin Landa
http://geo.fsv.cvut.cz/gwiki/Landa
http://gismentors.cz/mentors/landa
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Martin Landa]

Hi,

2015-10-05 9:41 GMT+02:00 Martin Landa :
> compilation was simply failing on build server (Debian stable):
>
>> In file included from main.c:47:0:
>> global.h:28:29: error: unknown type name ‘vec_struct’
>> global.h:28:43: error: unknown type name ‘vec_struct’
>> global.h:31:8: error: unknown type name ‘mat_struct’
> ...

even GRASS is compiled with lapack and blas support. I checked config.h again:

/* define if LAPACK exists */
#define HAVE_LIBLAPACK 1

/* define if BLAS exists */
#define HAVE_LIBBLAS 1

Ma

-- 
Martin Landa
http://geo.fsv.cvut.cz/gwiki/Landa
http://gismentors.cz/mentors/landa
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Vaclav Petras]

On Mon, Oct 5, 2015 at 3:44 AM, Martin Landa  wrote:
>
> Hi,
>
> 2015-10-05 9:41 GMT+02:00 Martin Landa :
> > compilation was simply failing on build server (Debian stable):

It is working on Ubuntu and probably Fedora as well. Perhaps it is a local
issue.

> >
> >> In file included from main.c:47:0:
> >> global.h:28:29: error: unknown type name ‘vec_struct’
> >> global.h:28:43: error: unknown type name ‘vec_struct’
> >> global.h:31:8: error: unknown type name ‘mat_struct’
> > ...
>
> even GRASS is compiled with lapack and blas support. I checked config.h
again:
>
> /* define if LAPACK exists */
> #define HAVE_LIBLAPACK 1
>
> /* define if BLAS exists */
> #define HAVE_LIBBLAS 1

Then la.h should be included in gmath.h. Including it in modules in a
workaround, not the solution. There are three possible solutions:

1) remove la.h from gmath.h and declare that modules must include it (I
don't see a reason for this)

2) change la.h include in gmath.h to something else; perhaps #if
defined(...) && defined(...) is wrong (but I don't think so, seems to fit
with the standard)

3) find out why it is different on that server and think about the proper
fix afterwards (ideal)

I can't test it since I can't reproduce it.

Sorry for being -pedantic but I don't think we should leave the code messy.
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev

Re: [GRASS-dev] [GRASS-SVN] r66413 - grass-addons/grass7/vector/v.kriging

[Vaclav Petras]

On Sun, Oct 4, 2015 at 9:08 AM,  wrote:
>
> Author: neteler
> Date: 2015-10-04 06:08:45 -0700 (Sun, 04 Oct 2015)
> New Revision: 66413
>
> Modified:
>grass-addons/grass7/vector/v.kriging/local_proto.h
> Log:
> v.kriging: add missing la.h header
>
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 

Hi,

this shouldn't be necessary and same for r66404, gmath.h contains already
grass/la.h. Any possible compilation issue is an issue with configuration
(local or in GRASS) or with the gmath.h file. What was the reason for the
change?

Vaclav

https://trac.osgeo.org/grass/browser/grass/trunk/include/gmath.h#L27
https://trac.osgeo.org/grass/changeset/66404
https://trac.osgeo.org/grass/changeset/66413
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev