Opened 4 years ago

Closed 2 years ago

#3084 closed enhancement (fixed)

i.segment: allow more than 2147483647 cells

Reported by: mlennert Owned by: grass-dev@…
Priority: normal Milestone: 8.0.0
Component: Imagery Version: unspecified
Keywords: i.segment variable type Cc:
CPU: Unspecified Platform: Unspecified

Description

In iseg.h, notnullcells is defined as long. On Windows this has a range of –2147483648 through 2147483647.

We are working on a region that has over 7 billion pixels and so the nonnullcells variable overflows, becomes negative and i.segment fails with "insufficient number of non-null cells".

Two reflections:

  • Shouldn't this be unsigned ?
  • Maybe a long long would be safer, seeing that pixel numbers don't stop increasing.

Change History (13)

comment:1 in reply to:  description ; Changed 4 years ago by mmetz

Replying to mlennert:

In iseg.h, notnullcells is defined as long. On Windows this has a range of –2147483648 through 2147483647.

We are working on a region that has over 7 billion pixels and so the nonnullcells variable overflows, becomes negative and i.segment fails with "insufficient number of non-null cells".

Two reflections:

  • Shouldn't this be unsigned ?
  • Maybe a long long would be safer, seeing that pixel numbers don't stop increasing.

For nonnullcells, long long would be ok. But i.segment is not prepared to handle raster maps with more than 2147483647 cells, at least the region IDs would also need to be changed, or handled differently. Variables for region ID as well as the region ID output is 32 bit signed integer, so there will be another integer overflow, particularly because in the beginning each cell gets a unique region ID.

comment:2 in reply to:  1 ; Changed 4 years ago by mlennert

Milestone: 7.0.57.2.0
Summary: i.segment: notnullcells defined as long too limitedi.segment: allow more than 2147483647 cells
Type: defectenhancement

Replying to mmetz:

Replying to mlennert:

In iseg.h, notnullcells is defined as long. On Windows this has a range of –2147483648 through 2147483647.

We are working on a region that has over 7 billion pixels and so the nonnullcells variable overflows, becomes negative and i.segment fails with "insufficient number of non-null cells".

Two reflections:

  • Shouldn't this be unsigned ?
  • Maybe a long long would be safer, seeing that pixel numbers don't stop increasing.

For nonnullcells, long long would be ok. But i.segment is not prepared to handle raster maps with more than 2147483647 cells, at least the region IDs would also need to be changed, or handled differently. Variables for region ID as well as the region ID output is 32 bit signed integer, so there will be another integer overflow, particularly because in the beginning each cell gets a unique region ID.

Ok, so I'll retype the ticket to enhancement request and change the title to reflect the demand for i.segment to handle over 2147483647 cells. Other than changing variable types, does this entail any other changes ?

comment:3 in reply to:  2 Changed 4 years ago by mmetz

Replying to mlennert:

Replying to mmetz:

Replying to mlennert:

In iseg.h, notnullcells is defined as long. On Windows this has a range of –2147483648 through 2147483647.

We are working on a region that has over 7 billion pixels and so the nonnullcells variable overflows, becomes negative and i.segment fails with "insufficient number of non-null cells".

Two reflections:

  • Shouldn't this be unsigned ?
  • Maybe a long long would be safer, seeing that pixel numbers don't stop increasing.

For nonnullcells, long long would be ok. But i.segment is not prepared to handle raster maps with more than 2147483647 cells, at least the region IDs would also need to be changed, or handled differently. Variables for region ID as well as the region ID output is 32 bit signed integer, so there will be another integer overflow, particularly because in the beginning each cell gets a unique region ID.

Ok, so I'll retype the ticket to enhancement request and change the title to reflect the demand for i.segment to handle over 2147483647 cells. Other than changing variable types, does this entail any other changes ?

Most importantly, a GRASS raster format for 64-bit signed integer. Note that GDAL does not support 64-bit signed or unsigned integers. The reason is probably that a portable implementation of 64-bit integers is not so easy. Regarding GRASS raster processing, the need for 64-bit integers usually arises only for raster maps with more than 2,147,483,647 cells which in turn usually requires Large File Support (LFS). Therefore the check for the availability of a 64-bit integer could be coupled to LFS. If support for 64-bit signed integer raster maps (say, LCELL) could be added to GRASS, users would need to stick to GRASS since GDAL raster export of 64-bit integers is not available. Interesting idea. In r68944, I have implemented for i.segment conditional support for 64-bit signed integers: long long int or a 64-bit off_t or a 64-bit long int must be available.

r68944 does not solve but alleviates the problem if the current region comprises more than 2,147,483,647 cells. r68944 also adds checks for integer overflow. The handling of initial region IDs has been changed and the module is now about 6x faster for regions > 10 million cells, depending on the threshold value for region growing.

comment:4 Changed 4 years ago by cmbarton

Just a comment about down-the-line issues. Using 64bit versions of dependencies like GDAL underscores the importance of getting the GUI to 64bit so we don't have to deal with the messiness of compiling dual architecture. This means moving the GUI to wxPython 3.x or wxPython Phoenix. IIRC, this is an issue for Windows as well as Mac currently. There is a test version of GRASS 7.3 64bit with wxPython 3 available now. It seems like there are not many issues to fix, but testing is needed to ID all of them. I don't know where we are with Windows on this. Anna is looking into moving to the Phoenix build but as of May, some of the GUI controls we need are still missing from this build.

comment:5 Changed 3 years ago by neteler

See also #2535

comment:6 Changed 3 years ago by neteler

Milestone: 7.2.07.2.1

Ticket retargeted after milestone closed

comment:7 Changed 3 years ago by martinl

Milestone: 7.2.17.2.2

comment:8 Changed 3 years ago by martinl

Milestone: 7.2.27.4.0

All enhancement tickets should be assigned to 7.4 milestone.

comment:9 Changed 2 years ago by neteler

Milestone: 7.4.07.4.1

Ticket retargeted after milestone closed

comment:10 Changed 2 years ago by martinl

What is status of this issue?

comment:11 in reply to:  10 Changed 2 years ago by mmetz

Replying to martinl:

What is status of this issue?

As of r68944, i.segment supports regions with more than 2,147,483,647 cells, but the number of final objects must not exceed 2,147,483,647. This can only be solved by introducing a new 64 bit integer data type in addition to CELL, FCELL, DCELL, something for GRASS 8 maybe.

comment:12 Changed 2 years ago by martinl

Milestone: 7.4.18.0.0

comment:13 Changed 2 years ago by mlennert

Resolution: fixed
Status: newclosed

The original request was answered, so closing this ticket. I opened a new enhancement request for a 64-bit raster format: #3485.

Note: See TracTickets for help on using tickets.