[gdal-dev] 2.2.0beta1 regression NULL values

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

[gdal-dev] 2.2.0beta1 regression NULL values

Roger Bivand
See https://github.com/edzer/sfr/issues/309 - < 2.2.0 we could write
"missing values" in vector drivers and read them back. In 2.2.0beta1, we
see REAL "missing values" written out OK, but read back as numeric zero.
We've only checked shapefiles and GPKG so far.

Is this RFC 67? Until now, rgdal code (and likely sf) has used:

       case OFTReal:
  if (poFeature->IsFieldSet(iField))
           REAL(ans)[iRow]=poFeature->GetFieldAsDouble(iField);
  else REAL(ans)[iRow]=NA_REAL;
  break;

and the same for other field and list field types. Should we be using:

int    CPL_DLL OGR_F_IsFieldSetAndNotNull( OGRFeatureH, int );

and should we be writing:

void   CPL_DLL OGR_F_SetFieldNull( OGRFeatureH, int );

instead of leaving the field unset with for example:

              if (!ISNA(NUMERIC_POINTER(VECTOR_ELT(ldata, j))[i]))
                  poFeature->SetField( CHAR(STRING_ELT(fld_names, j)),
                      NUMERIC_POINTER(VECTOR_ELT(ldata, j))[i] );

that is jumping over features for which the field value coming from R is
missing?

We'd need to condition on GDAL version here (too), I guess.

Was this regression intended?

Best wishes,

Roger

--
Roger Bivand
Department of Economics, Norwegian School of Economics,
Helleveien 30, N-5045 Bergen, Norway.
voice: +47 55 95 93 55; e-mail: [hidden email]
Editor-in-Chief of The R Journal, https://journal.r-project.org/index.html
http://orcid.org/0000-0003-2392-6140
https://scholar.google.no/citations?user=AWeghB0AAAAJ&hl=en
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Roger Bivand
NHH Norwegian School of Economics, Bergen, Norway
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2.2.0beta1 regression NULL values

Even Rouault-2

Hi Roger,

 

> See https://github.com/edzer/sfr/issues/309 - < 2.2.0 we could write

> "missing values" in vector drivers and read them back. In 2.2.0beta1, we

> see REAL "missing values" written out OK, but read back as numeric zero.

 

Did you check the output with ogrinfo/GDAL 2.2 to verify that they are indeed properly written as missing ?

 

> We've only checked shapefiles and GPKG so far.

>

> Is this RFC 67?

 

Most likely

 

> Until now, rgdal code (and likely sf) has used:

>

> case OFTReal:

> if (poFeature->IsFieldSet(iField))

> REAL(ans)[iRow]=poFeature->GetFieldAsDouble(iField);

> else REAL(ans)[iRow]=NA_REAL;

> break;

>

> and the same for other field and list field types. Should we be using:

>

> int CPL_DLL OGR_F_IsFieldSetAndNotNull( OGRFeatureH, int );

 

Yes

 

>

> and should we be writing:

>

> void CPL_DLL OGR_F_SetFieldNull( OGRFeatureH, int );

>

> instead of leaving the field unset with for example:

>

> if (!ISNA(NUMERIC_POINTER(VECTOR_ELT(ldata, j))[i]))

> poFeature->SetField( CHAR(STRING_ELT(fld_names, j)),

> NUMERIC_POINTER(VECTOR_ELT(ldata, j))[i] );

>

> that is jumping over features for which the field value coming from R is

> missing?

 

Leaving the field unset or setting it to Null with OGR_F_SetFieldNull() will have the same effect for most drivers. For example the shapefile driver uses IsFieldSetAndNotNull() internally on the write side, so both should result in the same result. The difference between unset and setting to null will only be seen for JSon-based drivers or GML.

 

>

> We'd need to condition on GDAL version here (too), I guess.

>

> Was this regression intended?

 

There are indeed a few backward incompatible changes (that you've mentionned above) per https://trac.osgeo.org/gdal/wiki/rfc67_nullfieldvalues that require user code changes.

 

Besides those intended changes, it is not impossible of course there's a real regression in GDAL itself, but ideally you should try to isolate it from the Rgdal code, possibly through a standalone C code or a GDAL Python script, so it can be more easily investigated (unfortunately, I couldn't really make sense of the ticket you mentionned due to my unfamiliarity with rgdal or sf). I'd also start by making sure where the issue is really: on the read side, or the write side. For example by producing a shapefile/GPKG with rgdal/GDAL 2.1.3 and reading it with rgdal/GDAL 2.2, and the reverse.

 

Best regards,

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2.2.0beta1 regression NULL values

Roger Bivand
Hi Even,

On Fri, 21 Apr 2017, Even Rouault wrote:

> Hi Roger,
>
>> See https://github.com/edzer/sfr/issues/309 - < 2.2.0 we could write
>> "missing values" in vector drivers and read them back. In 2.2.0beta1, we
>> see REAL "missing values" written out OK, but read back as numeric zero.
>
> Did you check the output with ogrinfo/GDAL 2.2 to verify that they are indeed properly
> written as missing ?
>
>> We've only checked shapefiles and GPKG so far.
>>
>> Is this RFC 67?
>
> Most likely
>
>> Until now, rgdal code (and likely sf) has used:
>>
>>        case OFTReal:
>>   if (poFeature->IsFieldSet(iField))
>>            REAL(ans)[iRow]=poFeature->GetFieldAsDouble(iField);
>>   else REAL(ans)[iRow]=NA_REAL;
>>   break;
>>
>> and the same for other field and list field types. Should we be using:
>>
>> int    CPL_DLL OGR_F_IsFieldSetAndNotNull( OGRFeatureH, int );
>
> Yes
>
>>
>> and should we be writing:
>>
>> void   CPL_DLL OGR_F_SetFieldNull( OGRFeatureH, int );
>>
>> instead of leaving the field unset with for example:
>>
>>               if (!ISNA(NUMERIC_POINTER(VECTOR_ELT(ldata, j))[i]))
>>                   poFeature->SetField( CHAR(STRING_ELT(fld_names, j)),
>>                       NUMERIC_POINTER(VECTOR_ELT(ldata, j))[i] );
>>
>> that is jumping over features for which the field value coming from R is
>> missing?
>
> Leaving the field unset or setting it to Null with OGR_F_SetFieldNull()
> will have the same effect for most drivers. For example the shapefile
> driver uses IsFieldSetAndNotNull() internally on the write side, so both
> should result in the same result. The difference between unset and
> setting to null will only be seen for JSon-based drivers or GML.

Have commited code changes for rgdal which now work the same for
2.2.0beta1 and 2.1.3 both for files generated by GDAL version, and crossed
(read 2.2.0beta1-generated files on 2.1.3 and vice-versa). Edzer made the
changes a week ago in sf.

>
>>
>> We'd need to condition on GDAL version here (too), I guess.
>>
>> Was this regression intended?
>
> There are indeed a few backward incompatible changes (that you've
> mentionned above) per
> https://trac.osgeo.org/gdal/wiki/rfc67_nullfieldvalues that require user
> code changes.

Not a regression as far as I can now see, just a consequence of the
"backward incompatible changes" that will affect many downstream users of
many drivers, I guess.

Thanks for getting back so quickly,

Roger

>
> Besides those intended changes, it is not impossible of course there's a
> real regression in GDAL itself, but ideally you should try to isolate it
> from the Rgdal code, possibly through a standalone C code or a GDAL
> Python script, so it can be more easily investigated (unfortunately, I
> couldn't really make sense of the ticket you mentionned due to my
> unfamiliarity with rgdal or sf). I'd also start by making sure where the
> issue is really: on the read side, or the write side. For example by
> producing a shapefile/GPKG with rgdal/GDAL 2.1.3 and reading it with
> rgdal/GDAL 2.2, and the reverse.
>
> Best regards,
>
> Even
>
>

--
Roger Bivand
Department of Economics, Norwegian School of Economics,
Helleveien 30, N-5045 Bergen, Norway.
voice: +47 55 95 93 55; e-mail: [hidden email]
Editor-in-Chief of The R Journal, https://journal.r-project.org/index.html
http://orcid.org/0000-0003-2392-6140
https://scholar.google.no/citations?user=AWeghB0AAAAJ&hl=en
_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Roger Bivand
NHH Norwegian School of Economics, Bergen, Norway
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2.2.0beta1 regression NULL values

Even Rouault-2

>

> Not a regression as far as I can now see, just a consequence of the

> "backward incompatible changes" that will affect many downstream users of

> many drivers, I guess.

 

When this was implemented, I didn't foresee a solution that would have kept a clean API while maintaining backward compatible behaviour, so yes that's the price to pay for new capabilities.

 

I'll modify the NEWS to mention in the RFC 67 item that there are backward incompatible change, and I'll actually fix MIGRATION_GUIDE.TXT since the related topic is actually misplaced in the MIGRATION GUIDE FROM GDAL 2.0 to GDAL 2.1 section, instead of 2.1 to 2.2

 

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
gdal-dev mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/gdal-dev
Loading...