search

ubarsc / rios

A raster processing layer on top of GDAL
https://www.rioshome.org
GNU General Public License v3.0
15 stars 8 forks source link

cleanup rat module and remove checks for ancient GDAL versions #26

Closed gillins closed 3 years ago

gillins commented 3 years ago

This PR attempts to tidy up the parts of the code that deal with pre-rfc40 (https://gdal.org/development/rfc/rfc40_enhanced_rat_support.html) versions of GDAL. Since this was introduced in GDAL 2.0 we feel that we no longer need to handle this.

As a result the color table code can use the new RAT reading and writing routines and these will be much faster for large color tables.

Things still to do on this PR:

neilflood commented 3 years ago

The checks for RFC40 can go, I agree that this is no longer required, given how old GDAL-2 is.

On the colour table changes, I am very strongly opposed to anything which changes the API. If I understand what you have changed, the changes to the colour table array, to only accept the 4-column form, would break any existing code which used the 5-column form, and certainly such code does exist.

neilflood commented 3 years ago

The suggestion to transpose the colour table array would also break every application which uses that API. I am not sure what benefit would arise from the change, so I am also opposed to that.

petebunting commented 3 years ago

I'm happy with the change to remove RFC40 checks. I would agree with Neil it is best not break existing code - although I don't think we use the colour table function in RIOS so probably wouldn't be a problem for us.

gillins commented 3 years ago

Thanks for comments guys. So there is code out there that actually manipulates values inside the table? I guess I'd always just used getColorTable or genColorTable to get the table before setting it, but I'm getting the sense this might not always be the case...

Regarding the axes of the table - the current format is optimised for setting row by row (via the ColorTable api) but I wanted something that was optimised for setting column by column (for use by RFC40 functions) to make it as fast as possible.

Also the 5 column form of the table doesn't work with RFC40 as we can't (easily) support missing rows, out of order etc.

How about I create a new colortable module with functions that deal with the table in the best way for RFC40 functions and leave the current functions as they are (and maybe deprecate them later).?

neilflood commented 3 years ago

re: colour tables. Yes. In RSC, there were a number of products in which the thematic values were not contiguous. For example, in their clearing rasters, values from 31-40 meant something, 41-59 were reserved for something else, 100-110 were reserved for other things, but there were large gaps in between, and not every value in those blocks was used every year. The code to manage these was written to create the colour table explicitly, with colours which would be meaningful to the editing staff, and so made use of the 5-column format, just setting those values it needed. Obviously it can be written differently, but the point is that there is code out there which will break if this change is made.

Very small colour tables were also easily created using statements like

tbl = numpy.array([
    [1, 80, 120, 150, 255],
    [2, 180, 90, 30, 255],
    [3, 70, 200, 50, 255]
])

The order of the axes is also one of the things making the above particularly clear. Again, it can all be written differently, but existing code should not be broken.

Not all colour tables are from the sort of voluminous segmentID-style rasters which RFC40 was designed for.

I am quite happy for you to create some new functions, though. Feel free.

gillins commented 3 years ago

Ah right - I hadn't seen code like that before. OK will keep those functions.

New functions in the rat module or a new colortable module? I'm tempted to have a new module as it is a distinct thing and there will be a lot of data for building the various standard colour tables that would be nice to have separate...

neilflood commented 3 years ago

re: a separate colortable module. I don't have strong feelings, I guess it depends on how much you are planning to add to it. I suggest you do it as you see fit, in a separate module, and if it upsets me I can ask you to shift the contents back to rat.

Perhaps a separate PR, as this one seems to be getting bigger? Up to you....

gillins commented 3 years ago

OK I have put get/setColorTable() back to the way they are. Will do the new colortable functions in a separate PR as you suggest.

I'll do some more testing of this tomorrow

gillins commented 3 years ago

I think this one is ready to merge Neil...

neilflood commented 3 years ago

Looks good to me. I didn't do any further testing, but everything looked as I would expect.

Thanks Neil

On Fri, 5 Feb 2021, at 1:57 PM, Sam Gillingham wrote:

I think this one is ready to merge Neil...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ubarsc/rios/pull/26#issuecomment-773770212, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3RD2I4AM2H2Y552WPH4C3S5NUCJANCNFSM4W4DJQ2Q.

-- Neil Flood neilflood@fastmail.fm