[GRASS-dev] [GRASS GIS] #2409: last call for options keys consolidation

GDAL · November 27, 2014, 5:14pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by martinl):

Replying to [comment:55 wenzeslaus]:

> > with lookup table `old_key:new_key` we can quite easily fix all
scripts and manual pages. Also parser will understand `old_key` will
continue to work. We were planned to do cleanup some years ago, this
ticket has been open 3 months ago. I believe that it will has good
consequences at the end. We just need to finish the job. That's all.

done in r63209 (feel free to update
source:grass/trunk/lib/gis/renamed_options which would be very valuable to
avoid large consequences you mentioned), eg.

{{{
d.rast map=dmt vallist=500-600
WARNING: Please update the interface of the module: option <vallist> has
been renamed to <values>
}}}

[...]

> This is not user friendly and thus it goes against the original
motivation for the change.

Sorry, I don't understand. Do you think that it's more user friendly when
`d.rast` fails with

{{{
ERROR: Sorry, <vallist> is not a valid parameter
}}}

?

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:56>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 27, 2014, 5:18pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by wenzeslaus):

Replying to [comment:56 martinl]:
> Replying to [comment:55 wenzeslaus]:
>
> > > with lookup table `old_key:new_key` we can quite easily fix all
scripts and manual pages. Also parser will understand `old_key` will
continue to work. We were planned to do cleanup some years ago, this
ticket has been open 3 months ago. I believe that it will has good
consequences at the end. We just need to finish the job. That's all.
>
> done in r63209 (feel free to update
source:grass/trunk/lib/gis/renamed_options which would be very valuable to
avoid large consequences you mentioned), eg.
>
> {{{
> d.rast map=dmt vallist=500-600
> WARNING: Please update the interface of the module: option <vallist> has
been renamed to <values>
> }}}
>
> [...]
>
> > This is not user friendly and thus it goes against the original
motivation for the change.
>
> Sorry, I don't understand. Do you think that it's more user friendly
when `d.rast` fails with
>
{{{
ERROR: Sorry, <vallist> is not a valid parameter
}}}
>
> ?

I was referring to rename of `rast3d` to `volume`. The `old_key` solves
the backward compatibility issue but not the issue of two different names
for one thing in manual which you created by introducing `volume`.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:57>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 27, 2014, 5:23pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by martinl):

Replying to [comment:51 wenzeslaus]:

> > shortly saying I don't see any problem to name the element as 'volume'
and use "3D raster map" in the manual and as element description..
> >
> I do.

OK, what about?

{{{
rast3d -> 3draster
region3d -> to be removed (?)
}}}

{{{
        type Data type(s)
               options:
raster,3draster,vector,oldvector,asciivector,labels,
                        region,region3d,group,all
                raster: raster map(s)
                3draster: 3D raster map(s)
                vector: vector map(s)
                oldvector: old vector map(s) (GRASS 5.0)
                asciivector: ASCII vector map(s)
                labels: paint label file(s)
                region: region definition(s)
                group: imagery group(s)
                all: all types
}}}

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:58>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 27, 2014, 5:39pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by neteler):

Replying to [comment:58 martinl]:
> OK, what about?
>
> {{{
> rast3d -> 3draster
> region3d -> to be removed (?)
> }}}

Sounds like a good compromise! +1

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:59>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 27, 2014, 5:45pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by annakrat):

If you use 3draster in python as module option it won't work.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:60>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 27, 2014, 5:51pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by martinl):

Replying to [comment:60 annakrat]:
> If you use 3draster in python as module option it won't work.

{{{
>>> g.read_command('g.list', type='3draster').splitlines()
['x']
}}}

?

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:61>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 27, 2014, 5:57pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by mlennert):

Just a heads up for this discussion and the actual implementation:

r63102 broke v.db.update because the key was changed in the parameter
definitions, but the use of the key wasn't:

qcolumn key changed to query_column, but this was not done for the parsing
of the parameter content:

qcolumn = options['qcolumn']

I corrected it in r63211 and r63212, but please watch out for this.

Moritz

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:62>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 27, 2014, 6:11pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by glynn):

Replying to [comment:32 annakrat]:
> We shouldn't forget that although unabbreviating doesn't cause problems
in the command line, python scripts require the full name of the command.
PyGRASS may require that; grass.script.run_command() etc don't.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:63>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 28, 2014, 2:12am

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by annakrat):

Replying to [comment:61 martinl]:
> Replying to [comment:60 annakrat]:
> > If you use 3draster in python as module option it won't work.
>
> {{{
> >>> g.read_command('g.list', type='3draster').splitlines()
> ['x']
> }}}
>
> ?

I wrote 'as module option'. If you will make change to the type, you
should make the change to the option name too and this will not work:

{{{
>>> gscript.run_command('g.region', 3draster='xxx')
  File "<stdin>", line 1
    gscript.run_command('g.region', 3draster='xxx')
                                           ^
SyntaxError: invalid syntax
}}}

But otherwise, I like this better than 'volume', so perhaps we could find
some workaround for Python scripts and use underscore:
{{{
>>> gscript.run_command('g.region', _3draster='xxx')
}}}

This should work, although the underscore before was marked as obsolete,
in favor of underscore after (for python keywords like map, input). But we
can make an exception for keywords starting with numbers.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:64>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 28, 2014, 8:24am

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by martinl):

Replying to [comment:62 mlennert]:
> I corrected it in r63211 and r63212, but please watch out for this.

thanks anyway I am going to fix scripts and manuals using a script based
on source:grass/trunk/lib/gis/renamed_options.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:65>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 28, 2014, 8:31am

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by martinl):

Replying to [comment:64 annakrat]:

> But otherwise, I like this better than 'volume', so perhaps we could
find some workaround for Python scripts and use underscore:
> {{{
> >>> gscript.run_command('g.region', _3draster='xxx')
> }}}
>
> This should work, although the underscore before was marked as obsolete,
in favor of underscore after (for python keywords like map, input). But we
can make an exception for keywords starting with numbers.

OK, I changed element name from 'volume' to '3draster' in r63218.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:66>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 28, 2014, 10:10am

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by martinl):

Replying to [comment:45 martinl]:
> Replying to [comment:44 martinl]:
>
> > in r63189 I renamed
>
> btw, do we need `region3d` ? ASAIK the region is 3D by default. Martin

removed in r63236. Now we have

{{{
                raster: raster map(s)
                3draster: 3D raster map(s)
                vector: vector map(s)
                oldvector: old (GRASS 5.0) vector map(s)
                asciivector: ASCII vector map(s)
                labels: paint label file(s)
                region: region definition(s)
                group: imagery group(s)
                all: all types
}}}

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:67>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 28, 2014, 1:51pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by huhabla):

I am not against renaming the options, but IMHO its completely
unnecessary. However, if the majority thinks it is necessary then i am
fine with that.

But the recent changes in the option naming broke the temporal framework
and as a result many temporal modules and tests.

Why??!!

If such invasive changes are performed, then i would expect that all the
consequences of these changes are recognized and fixed before the commit!

We have a wonderful testsuite, so please use it to detect possible issues.
Please do not simply break GRASS and expect that other developers will fix
it.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:68>
GRASS GIS <http://grass.osgeo.org>

mlennert · November 28, 2014, 2:05pm

On 28/11/14 14:51, GRASS GIS wrote:

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
  Reporter: martinl | Owner: grass-dev@…
      Type: task | Status: new
  Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
  Keywords: standardized options | Platform: Unspecified
       Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by huhabla):

  I am not against renaming the options, but IMHO its completely
  unnecessary. However, if the majority thinks it is necessary then i am
  fine with that.

  But the recent changes in the option naming broke the temporal framework
  and as a result many temporal modules and tests.

  Why??!!

  If such invasive changes are performed, then i would expect that all the
  consequences of these changes are recognized and fixed before the commit!

  We have a wonderful testsuite, so please use it to detect possible issues.
  Please do not simply break GRASS and expect that other developers will fix
  it.

Yes, I've already stumbled upon one test which fails because of changed type name (r.slope.aspect test calls g.remove with type="rast").

I agree with Soeren that we have to be careful about these changes, especially if we want to release in a not too far future...

Moritz

wenzeslaus · November 28, 2014, 2:27pm

On Fri, Nov 28, 2014 at 9:05 AM, Moritz Lennert <
mlennert@club.worldonline.be> wrote:

On 28/11/14 14:51, GRASS GIS wrote:

#2409: last call for options keys consolidation
----------------------------------+-------------------------
----------------
  Reporter: martinl | Owner: grass-dev@…
      Type: task | Status: new
  Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
  Keywords: standardized options | Platform: Unspecified
       Cpu: Unspecified |
----------------------------------+-------------------------
----------------

Comment(by huhabla):

  I am not against renaming the options, but IMHO its completely
  unnecessary. However, if the majority thinks it is necessary then i am
  fine with that.

  But the recent changes in the option naming broke the temporal framework
  and as a result many temporal modules and tests.

  Why??!!

  If such invasive changes are performed, then i would expect that all the
  consequences of these changes are recognized and fixed before the
commit!

  We have a wonderful testsuite, so please use it to detect possible
issues.
  Please do not simply break GRASS and expect that other developers will
fix
  it.

Yes, I've already stumbled upon one test which fails because of changed
type name (r.slope.aspect test calls g.remove with type="rast").

I agree with Soeren that we have to be careful about these changes,
especially if we want to release in a not too far future...

To run all tests before a commit run:

python -m grass.gunittest.main --location nc_spm_grass7 --location-type nc

inside GRASS session. nc_spm_grass7 should be the name of NC sample
location in your current GISDBASE (grassdata). The command will also work
in any subdirectory.

To run just one test file, cd to testsuite directory with the test file and
run

python test_something.py

inside GRASS session. This will use the current location and mapset.

See more at:

http://grass.osgeo.org/grass71/manuals/libpython/gunittest_testing.html
http://grass.osgeo.org/grass71/manuals/libpython/gunittest_running_tests.html

To see the results of tests which currently run once a day on a server, see:

http://fatra.cnr.ncsu.edu/grassgistests/

The most interesting is the report for nc location:

http://fatra.cnr.ncsu.edu/grassgistests/summary_report/nc/index.html

Which currently ends with:
...
2014-11-26 03:01:42 62954 nc_spm_08_grass7 85% 98%
2014-11-27 03:01:47 63174 nc_spm_08_grass7 38% 80%
2014-11-28 03:01:44 63217 nc_spm_08_grass7 39% 81%

The recent report for individual test files is:

http://fatra.cnr.ncsu.edu/grassgistests/reports_for_date-2014-11-28-08-00/report_for_nc_spm_08_grass7_nc/testfiles.html

When running the test yourself, you will get the same output as you can see
online for one day. So, your index.html should look like:

http://fatra.cnr.ncsu.edu/grassgistests/reports_for_date-2014-11-28-08-00/report_for_nc_spm_08_grass7_nc/index.html

You will find this index.html file in a testreport directory after running
the tests using `python -m grass.gunittest.main ...`.

Vaclav

Moritz

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

GDAL · November 28, 2014, 3:06pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by martinl):

Replying to [comment:68 huhabla]:

> If such invasive changes are performed, then i would expect that all the
consequences of these changes are recognized and fixed before the commit!

I will fix it today.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:69>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 28, 2014, 3:32pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by wenzeslaus):

Replying to [comment:69 martinl]:
> Replying to [comment:68 huhabla]:
>
> > If such invasive changes are performed, then i would expect that all
the consequences of these changes are recognized and fixed before the
commit!
>
> I will fix it today.

Before doing so, I would suggest to re-think the change, especially the
one for 3D raster. We don't have an agreement and I even don't see what
you are currently trying to implement:

{{{
> d.legend --h 2>&1 | grep 3D
raster3d Name of 3D raster map
> g.region --h 2>&1 | grep 3D
3draster Set region to match 3D raster map(s) (both 2D and 3D values)
res3 3D grid resolution (north-south, east-west and top-bottom)
}}}

Moreover, I'm not sure what do you want to do with module names such as
`d.rast`, `r.to.rast3` and all temporal datatypes.

Please, react also to the notes made by [comment:34 me], [comment:54 Anna]
and [comment:68 Soeren] about the need for the change and the motivation
behind the decisions for change of `rast3d` to `raster3d/3draster/volume`.
From what was said I would especially point out that it seems to me that
possibility to shorten the options is valued much more than documentation
consistency and Python interface.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:70>
GRASS GIS <http://grass.osgeo.org>

GDAL · November 28, 2014, 4:27pm

#2409: last call for options keys consolidation
----------------------------------+-----------------------------------------
Reporter: martinl | Owner: grass-dev@…
Type: task | Status: new
Priority: blocker | Milestone: 7.0.0
Component: Default | Version: unspecified
Keywords: standardized options | Platform: Unspecified
Cpu: Unspecified |
----------------------------------+-----------------------------------------

Comment(by wenzeslaus):

Martin and Markus, maybe I should add, just to clarify, that I'm
definitively in favor of all other changes you are doing such as `dsn ->
output`, `output_prefix -> output`, `devi -> deviations` and
`minimumtemperature -> minimum_temperature`. I'm just very uncomfortable
with changes related to rast/vect/rast3d, what you then imply for 3D
raster and that you don't consider also module names.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/2409#comment:71>
GRASS GIS <http://grass.osgeo.org>

neteler · November 28, 2014, 4:37pm

Hi devs

please avoid hammering on Martin.

He had been doing a tremendous job this week. The parameter sync was overdue. The remaining issues will be sorted out, don’t worry.

Thanks for his hard work

Markus

wenzeslaus · November 28, 2014, 5:12pm

You are right, Markus.

Sorry about that, Martin, and thank you for all the changes you have done.

Please, keep all of us in the loop with updates on the remaining issues.

Vaclav

···

On Fri, Nov 28, 2014 at 11:37 AM, Markus Neteler <neteler@osgeo.org> wrote:

Hi devs

please avoid hammering on Martin.

He had been doing a tremendous job this week. The parameter sync was overdue. The remaining issues will be sorted out, don’t worry.

Thanks for his hard work

Markus

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