Breaking: Standardise cf standards and missingval handling #695

rafaqz · 2024-07-12T12:29:51Z

This PR standardises cf and missingval behaviour accross all sources.

New keywords are:

cf=true: apply offset and scale if the are not zero and one.
maskingval=missing this is the value the missingval will be converted to if there is one.

I don't like either of these names! if you can think of anything better please suggest something.

@felixcremer @meggart this is the new diskarrays object that does 2-way cf and missingval/maskingval conversions

https://github.com/rafaqz/Rasters.jl/compare/cf?expand=1#diff-1478a88a8bdcffcf7f556cc1865dbe18d69b5c03f75b4481e1f5889e8a6b6b73

Rapsodia86 · 2024-07-28T00:57:55Z

How about inverting it? Instead of "cf" call it "raw". If raw=false, offset and scale are applied, if raw=true, offset and scale are NOT applied?

And raw=true would be the default?

Maybe "navalue" instead of "maskingval"?

rafaqz · 2024-07-28T14:41:41Z

Currently I've got scaled=true as the default instead of cf. raw might suggest the missing value is also not transformed?

Main concern with navalue is it sounds like R, and isn't really used in julia

Rapsodia86 · 2024-07-28T16:22:51Z

Yes, you are right that with "raw" one may expect that data were not touched at all.

Indeed, "navalue" originates from my heavy R usage! And I agree NA is not very julia way.

Point taken:)

rafaqz · 2024-07-28T16:26:57Z

Thanks for the input though, good to have all the options to choose from

rafaqz · 2024-08-13T18:35:07Z

Coming back to this I like how short and obvious raw=true is!

But to mean no scaling and no masking - so it will override everything else.

codecov · 2024-08-17T01:57:37Z

Codecov Report

Attention: Patch coverage is 82.84251% with 134 lines in your changes missing coverage. Please review.

Project coverage is 81.79%. Comparing base (570fd17) to head (54056cb).
Report is 9 commits behind head on main.

Files	Patch %	Lines
src/modifieddiskarray.jl	68.61%	43 Missing ⚠️
src/utils.jl	80.48%	32 Missing ⚠️
ext/RastersZarrDatasetsExt/zarrdatasets_source.jl	0.00%	13 Missing ⚠️
...imensionalDataExt/GeometryOpsDimensionalDataExt.jl	0.00%	8 Missing ⚠️
ext/RastersArchGDALExt/gdal_source.jl	89.55%	7 Missing ⚠️
src/stack.jl	85.71%	7 Missing ⚠️
src/write.jl	75.00%	7 Missing ⚠️
ext/RastersNCDatasetsExt/ncdatasets_source.jl	84.61%	4 Missing ⚠️
src/sources/commondatamodel.jl	92.85%	4 Missing ⚠️
src/create.jl	96.10%	3 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #695      +/-   ##
==========================================
- Coverage   82.59%   81.79%   -0.80%     
==========================================
  Files          60       64       +4     
  Lines        4510     4884     +374     
==========================================
+ Hits         3725     3995     +270     
- Misses        785      889     +104

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

asinghvi17 · 2024-09-01T22:14:00Z

ext/GeometryOpsDimensionalDataExt/GeometryOpsDimensionalDataExt.jl

@@ -0,0 +1,20 @@
+module GeometryOpsDimensionalDataExt


Should we close the PR that does this in GeometryOps?

No I must have added that by mistake lol

src/create.jl

asinghvi17 · 2024-09-01T22:18:54Z

src/create.jl


+"""
+    create([f!], [filename], template; kw...)


When should I use this as opposed to Raster(data, dims; ...)? What are the advantages beyond efficiency in writing to file?

Or is the direct constructor going to call create?

Well Raster(filename) reads an existing file... create(filename) creates one.

So this is best for building something new from scratch, Raster is best for opening something, rewrapping a DimArray, etc. Otherwise you need to define the Raster in-memory then write it as separate steps.

(create is already called in a lot of places under the hood where a new file might be created, like in nearly all methods with a filename keyword. It's been here for years, just not publicly)

Gotcha, I guess I'm not entirely clear on what to use when creating a purely in memory dataset. Does the pure Raster(data, dims; kwargs...) constructor have the same options? Should it? That's the most intuitive option IMO to create an in memory raster...

Yeah it will be pretty much identical for in-memory rasters. But if you want chunks and e.g. compression options you have to do it separately in write. An in-memory raster doesn't have chunks or options like that.

Sweet that sounds good. Will add a suggestion for a line in the docstring to clarify.

Co-authored-by: Anshul Singhvi <[email protected]>

This fixes a strange issue I was having where Rasters.jl read the wrong values from a Kerchunk/Zarr dataset.

asinghvi17 · 2024-09-11T23:32:39Z

src/methods/shared_docstrings.jl

+    To ignore `scale` and `offset` metadata, use `scaled=false`. If `scale` and
+    Note: `offset` are `1.0` and `0.0` they will be ignored and the original type will 


Suggested change

To ignore `scale` and `offset` metadata, use `scaled=false`. If `scale` and

Note: `offset` are `1.0` and `0.0` they will be ignored and the original type will

To ignore `scale` and `offset` metadata, use `scaled=false`. Note: If `scale`

and `offset` are `1.0` and `0.0` they will be ignored and the original type will

asinghvi17 · 2024-09-11T23:33:10Z

src/methods/shared_docstrings.jl

+
+const COERCE_KEYWORD = """
+- `coerce`: where `scale` and/or `offset` are present during `setindex!` to disk, 
+    coerce values to the disk type. `convert` is the default, but `round`, `trunc` or


What's the disk type?

Oh it should be eltype

asinghvi17 · 2024-09-11T23:35:26Z

src/methods/shared_docstrings.jl

+- `maskingval`: A value to convert `missingval` to, by default `missing`. If this is set it 
+    will be the return value of `missingval(raster)` - `maskingval` becomes the new `missingval`.
+    Setting `maskingval` to `nothing` means no masking will occur, and the original `missingval` 
+    will be the final `missingval`. This can give better performance than using `missing`. 
+    Another efficient option is to use e.g. `zero(eltype(raster))` to replace missing values with zero.


I can see why you picked this but I'm not totally happy about the name. Still, I can't see any other nice options. missingval would have been nice but it's already in use. I'd say we should go for it. A sufficiently good docstring will clarify all problems :P

Maybe emphasize that this does what replace_missing does?

Suggested change

- `maskingval`: A value to convert `missingval` to, by default `missing`. If this is set it

will be the return value of `missingval(raster)` - `maskingval` becomes the new `missingval`.

Setting `maskingval` to `nothing` means no masking will occur, and the original `missingval`

will be the final `missingval`. This can give better performance than using `missing`.

Another efficient option is to use e.g. `zero(eltype(raster))` to replace missing values with zero.

- `maskingval`: A value to convert `missingval` to, by default `missing`. If this is set it

will be the return value of `missingval(raster)` - `maskingval` becomes the new `missingval`.

This is the same thing that `replace_missing(raster, some_value)` does.

Setting `maskingval` to `nothing` means no masking will occur, and the original `missingval`

will be the final `missingval`. This can give better performance than using `missing`.

Another efficient option is to pass e.g. `zero(eltype(raster))` to replace missing values with zero.

It's also what CommonDataModel uses so there's precedent... But sharing the mask part with the mask function is kinda weird for me

asinghvi17 · 2024-09-11T23:36:24Z

src/methods/shared_docstrings.jl

+
+const WRITE_MISSINGVAL_KEYWORD = """
+- `missingval`: set the missing value (i.e. FillValue / nodataval) of the written raster,
+    as Julias `missing` cannot be stored. If not passed in, `missingval` will be detected 


Suggested change

as Julias `missing` cannot be stored. If not passed in, `missingval` will be detected

as Julia's `missing` cannot be stored. If not passed in, `missingval` will be detected

rafaqz added 2 commits July 12, 2024 14:23

start adding cf keyword

ec02fd1

add modifieddiskarray file

5f7bed4

rafaqz changed the title Cf Standardise cf standards and missingval handling Jul 12, 2024

rafaqz added 3 commits July 14, 2024 23:17

use scale and offset keywords

8be3f25

bugfix

ded3eeb

bugfix

9e6019c

rafaqz changed the title ~~Standardise cf standards and missingval handling~~ Breaking: Standardise cf standards and missingval handling Jul 15, 2024

rafaqz added the breaking label Jul 15, 2024

rafaqz added 3 commits July 17, 2024 14:08

Merge branch 'main' into cf

53e167b

updates

afe7920

create

967083c

rafaqz mentioned this pull request Jul 26, 2024

Feature request: mask_and_scale #702

Open

rafaqz added 4 commits August 2, 2024 09:13

tweaks

c8a3828

bugfixes

7a87d81

bugfix GRIB

a7d9b19

bugfixes

ee50b63

add raw keyword

1a0c0d6

rafaqz force-pushed the cf branch from c7df0bc to 329e92e Compare August 13, 2024 21:04

rafaqz added 7 commits August 13, 2024 23:06

clean up extensions

ce3b152

updates

329e92e

mosaic fixes and standardisation

981dd69

bugfixes

b7e63f2

closured in create

1c20275

Merge branch 'main' into cf

53b16e3

bugfixes

a034c60

fix resample test for nokw

54056cb

test create with a function

6a57727

rafaqz mentioned this pull request Sep 1, 2024

[Documentation] Tutorial: Building a raster from scratch #720

Open

asinghvi17 reviewed Sep 1, 2024

View reviewed changes

rafaqz and others added 2 commits September 3, 2024 11:10

Apply suggestions from code review

8500e42

Co-authored-by: Anshul Singhvi <[email protected]>

Hook up scale and offset to Zarr datasets via CDM (#732)

c225840

This fixes a strange issue I was having where Rasters.jl read the wrong values from a Kerchunk/Zarr dataset.

asinghvi17 reviewed Sep 11, 2024

View reviewed changes

rafaqz mentioned this pull request Sep 19, 2024

Bump patch version, DD compat #750

Closed

rafaqz added 3 commits September 19, 2024 15:31

use coalesceval instead of maskingval

cfa1b95

merge changes

3d975e9

combine missingval and maskingval

5f9be64

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking: Standardise cf standards and missingval handling #695

Breaking: Standardise cf standards and missingval handling #695

rafaqz commented Jul 12, 2024

Rapsodia86 commented Jul 28, 2024 •

edited

Loading

rafaqz commented Jul 28, 2024

Rapsodia86 commented Jul 28, 2024

rafaqz commented Jul 28, 2024

rafaqz commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 17, 2024

asinghvi17 Sep 1, 2024 •

edited

Loading

rafaqz Sep 2, 2024 •

edited

Loading

asinghvi17 Sep 1, 2024

asinghvi17 Sep 1, 2024

rafaqz Sep 2, 2024 •

edited

Loading

asinghvi17 Sep 2, 2024

rafaqz Sep 2, 2024

asinghvi17 Sep 2, 2024

asinghvi17 Sep 11, 2024

asinghvi17 Sep 11, 2024

rafaqz Sep 12, 2024

asinghvi17 Sep 11, 2024

rafaqz Sep 12, 2024

asinghvi17 Sep 11, 2024

		To ignore `scale` and `offset` metadata, use `scaled=false`. If `scale` and
		Note: `offset` are `1.0` and `0.0` they will be ignored and the original type will

	as Julias `missing` cannot be stored. If not passed in, `missingval` will be detected
	as Julia's `missing` cannot be stored. If not passed in, `missingval` will be detected

Breaking: Standardise cf standards and missingval handling #695

Are you sure you want to change the base?

Breaking: Standardise cf standards and missingval handling #695

Conversation

rafaqz commented Jul 12, 2024

Rapsodia86 commented Jul 28, 2024 • edited Loading

rafaqz commented Jul 28, 2024

Rapsodia86 commented Jul 28, 2024

rafaqz commented Jul 28, 2024

rafaqz commented Aug 13, 2024 • edited Loading

codecov bot commented Aug 17, 2024

Codecov Report

asinghvi17 Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

rafaqz Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rapsodia86 commented Jul 28, 2024 •

edited

Loading

rafaqz commented Aug 13, 2024 •

edited

Loading

asinghvi17 Sep 1, 2024 •

edited

Loading

rafaqz Sep 2, 2024 •

edited

Loading

rafaqz Sep 2, 2024 •

edited

Loading