Opened 4 years ago

Closed 3 years ago

#3010 closed defect (fixed)

PyGRASS fails to write vector map with attributes

Reported by: annakrat Owned by: grass-dev@…
Priority: major Milestone: 7.2.0
Component: PyGRASS Version: svn-trunk
Keywords: vector, API break, backwards compatibility Cc:
CPU: Unspecified Platform: All

Description

Example taken from a workshop material:

from grass.pygrass.vector import VectorTopo
from grass.pygrass.vector.geometry import Point
point1 = Point(635818.8, 221342.4)
point2 = Point(633627.7, 227050.2)
cols = [(u'cat',       'INTEGER PRIMARY KEY'),
        (u'name',      'TEXT')]
 
with VectorTopo('my_points', mode='w', tab_cols=cols, overwrite=True) as my_points:
    # save the point and the attribute
    my_points.write(point1, ('pub', ))
    my_points.write(point2, ('restaurant', ))
    # save the changes to the database
    my_points.table.conn.commit()

gives:

WARNING: Vector map <my_points> already exists and will be overwritten
WARNING: Coor file of vector map <my_points@PERMANENT> is larger than it
         should be (18 bytes excess)
Building topology for vector map <my_points@PERMANENT>...
Registering primitives...
0 primitives registered
0 vertices registered
Building areas...
 100%
0 areas built
0 isles built
Attaching islands...
Attaching centroids...
Number of nodes: 0
Number of primitives: 0
Number of points: 0
Number of lines: 0
Number of boundaries: 0
Number of centroids: 0
Number of areas: 0
Number of isles: 0
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/home/anna/dev/grass/trunk1/dist.x86_64-pc-linux-gnu/etc/python/grass/pygrass/errors.py", line 15, in wrapper
    return method(self, *args, **kargs)
  File "/home/anna/dev/grass/trunk1/dist.x86_64-pc-linux-gnu/etc/python/grass/pygrass/vector/__init__.py", line 197, in write
    cats.set(cat, self.layer)
  File "/home/anna/dev/grass/trunk1/dist.x86_64-pc-linux-gnu/etc/python/grass/pygrass/vector/basic.py", line 452, in set
    libvect.Vect_cat_set(self.c_cats, layer, cat)
ctypes.ArgumentError: argument 3: <type 'exceptions.TypeError'>: wrong type

Works in 7.0.4.

Change History (10)

comment:1 Changed 4 years ago by neteler

Milestone: 7.1.07.2.0

Milestone renamed

comment:2 Changed 4 years ago by annakrat

Resolution: fixed
Status: newclosed

OK, the API changed in r66016 and the documentation was not updated. Fixed in r68603 and backported in r68604 to releasebranch 7.2.

The right syntax is now:

new.write(point0, cat=1, attrs=('pub',))

comment:3 Changed 3 years ago by wenzeslaus

Keywords: API break backwards compatibility added
Resolution: fixed
Status: closedreopened

The following from r68603 is breaking backwards compatibility between 7.0 and 7.2:

- def write(self, geo_obj, attrs=None, set_cats=True):
+ def write(self, geo_obj, cat=None, attrs=None):

The old API should (must) be accommodated. If the old API is so bad, that it needs to be replaced (rather than extended), then we should have one transitional version where both APIs work and the old one gives a warning. I don't have enough information to make the judgment.

Related to that, the error message does not give much information about what is going on.

comment:4 in reply to:  3 ; Changed 3 years ago by martinl

Replying to wenzeslaus:

The following from r68603 is breaking backwards compatibility between 7.0 and 7.2:

There are much more breakage of backward compatibility between 7.0 and 7.2 (speaking about pyGRASS). This is just one of them.

comment:5 in reply to:  3 ; Changed 3 years ago by huhabla

IMHO the former implementation was close to be a bug that was fixed in trunk. For every new feature a new category was created by default, except a category was already attached to the c_cat field in the geometry. It was not obvious howto use the same category for different features.

Replying to wenzeslaus:

The following from r68603 is breaking backwards compatibility between 7.0 and 7.2:

- def write(self, geo_obj, attrs=None, set_cats=True):
+ def write(self, geo_obj, cat=None, attrs=None):

The old API should (must) be accommodated. If the old API is so bad, that it needs to be replaced (rather than extended), then we should have one transitional version where both APIs work and the old one gives a warning. I don't have enough information to make the judgment.

Related to that, the error message does not give much information about what is going on.

comment:6 in reply to:  5 Changed 3 years ago by zarch

Replying to huhabla:

IMHO the former implementation was close to be a bug that was fixed in trunk. For every new feature a new category was created by default, except a category was already attached to the c_cat field in the geometry. It was not obvious howto use the same category for different features.

I do think that the current version is better than it was.

But it breaks things and this is why I've informally ask to Markus N. if it would be possible to have a grass addons directory grass70 and grass72... But he want to avoid to have further differentiation of the grass folders. Because the code is not portable between these two versions.

[off topic] Perhaps git would be an option where you use a dedicate branch for grass6/grass70/grass7x.

comment:7 in reply to:  5 Changed 3 years ago by wenzeslaus

Replying to huhabla:

IMHO the former implementation was close to be a bug that was fixed in trunk.

If it was buggy, than there should be a special if with an error message (exception, fatal, assert) which will tell you that you are using old API which was buggy and you need to change your code.

BTW, the current message is not that good even in context of the 7.2 API only because you don't know what has the wrong type.

For every new feature a new category was created by default, except a category was already attached to the c_cat field in the geometry.

This does not seem buggy but as a simple one which not flexible enough for advanced user but pretty good for new users.

It was not obvious howto use the same category for different features.

This sounds like a problem with documentation or a need for additional API, seems like a long jump to breaking backwards compatibility.

Don't take me wrong, I agree that the new API is needed. It just needs some better handling for the 7.0 API. (We are getting complains about breaking API for modules between 6.4 and 7.0, so I suppose users care about backwards compatibility.)

comment:8 Changed 3 years ago by wenzeslaus

In 69809:

pygrass: 7.0 API now works even without using kwargs for Vector.write(), see #3010

comment:9 in reply to:  4 Changed 3 years ago by wenzeslaus

Replying to martinl:

Replying to wenzeslaus:

The following from r68603 is breaking backwards compatibility between 7.0 and 7.2:

There are much more breakage of backward compatibility between 7.0 and 7.2 (speaking about pyGRASS). This is just one of them.

If there is more, it would be good to have them in wiki:Release/7.2.0-News#PyGRASSchanges.

For the parts where the feature is a basic feature (like writing attributes) or it is easy to implement (like switching parameters) we should keep compatibility like in r69809 (the cat auto-incrementing was already there).

How to report that to the user is a question. I used

import warnings
warnings.warn("Vector.write(geo_obj, attrs=(...)) is"
              " depreciated, specify cat explicitly",
              DeprecationWarning)

which is a more conservative option - by default it is visible only with python -Qwarn.

If OK, r69809 needs backport to 7.2.

comment:10 Changed 3 years ago by wenzeslaus

Resolution: fixed
Status: reopenedclosed

In 69832:

pygrass: 7.0 API now works even without using kwargs for Vector.write(), closes #3010

Note: See TracTickets for help on using tickets.