Skip to content

update nodata docs to show cell type conversion behavior and aggregations#230

Merged
vpipkt merged 10 commits into
locationtech:developfrom
s22s:feature/nodata-add-doc
Aug 7, 2019
Merged

update nodata docs to show cell type conversion behavior and aggregations#230
vpipkt merged 10 commits into
locationtech:developfrom
s22s:feature/nodata-add-doc

Conversation

@emc5ud

@emc5ud emc5ud commented Aug 1, 2019

Copy link
Copy Markdown
Contributor

Signed-off-by: Eric Culbertson eculbertson@astraea.io

Eric Culbertson added 2 commits August 1, 2019 16:04
Signed-off-by: Eric Culbertson <eculbertson@astraea.io>
Signed-off-by: Eric Culbertson <eculbertson@astraea.io>
@metasim
metasim requested a review from vpipkt August 1, 2019 21:03

@metasim metasim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for your contribution.

Are there areas where we can cross reference other sections of the documentation?

Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd Outdated
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd Outdated
Signed-off-by: Eric Culbertson <eculbertson@astraea.io>
Signed-off-by: Eric Culbertson <eculbertson@astraea.io>

@vpipkt vpipkt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great contributions. I have a few minor wording things to pick on and a few things to perhaps simplify the code examples.

It's also nice to view a sample.

```python show_masked
```python

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the chunk labels, they are somewhat helpful for interactive builds

But I suspect this is a response to #227 ....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right yeah I removed them because the text pyweave generates when calling display. I don't know the best way to handle that for now.

Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd
Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd

```python
masked_rf.select(rf_tile_sum('tile'), rf_tile_sum('tile_nd_1'), rf_tile_sum('tile_nd_2')).show()
```

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be worthwhile to perhaps add a level 3 header ### differentiating the Tile Aggregate, DataFrame Aggregate, and Local Aggregate behaviors. Here you have done Tile Agg, could expand to include the other 2 or just point to the aggreagation.md docs page

Another thought here it may be worth working through the artithmetic for one of the cases to show the reader how to work out the expected sum. it's a block of 25*100 * value for each, right.

FWIW also there is a new section we could add about ND in local algebra; the bug #229 is now fixed to rf_local_add(t1, t2) will work like d + nd = nd.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in that comment can be used as the example in the documentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpipkt I added back the section with the local sum and NoData. I'd like you to check to make sure that sum is working correctly.

Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd Outdated
rf_added.select(rf_cell_type('xy_sum'), rf_cell_type('y'), rf_cell_type('x')).distinct().show()
```

Combining tile columns of different cell types gets a little trickier when user defined NoData cell types are involved. Let's create 3 tile columns: one without a defined NoData value, one with a NoData value of 1, and one with a NoData value of 2.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first tile you should say one with the "default NoData value of zero" versus wihtout defined NoData. It's the distinction between uint16 (has nodata, nodata value is 0) and uint16raw (does not have a nodata) .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is the int16 does not have a NoData value of 0, while uint16 does

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but that function makes tiles with uint16 nvm

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpipkt I changed this section to remove the confusing between uint16 and int16.

Comment thread pyrasterframes/src/main/python/docs/nodata-handling.pymd Outdated
.withColumn('x_nd_2', rf_convert_cell_type('x', get_nodata_ct(2)))
```

Let's try adding the tile column without a defined NoData value to a tile column with a defined NoData value. When there is an inconsistent NoData value in the two columns, the NoData value of the right-hand side of the sum is kept. In this case, this means the result has no defined NoData value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again let's be careful with wording around "no defined NoData" versus "default NoData" ... if you're confused about it you can always use the CellType() methods has_no_data and no_data_value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpipkt I believe I made the wording more clear.

Signed-off-by: Jason T. Brown <jason@astraea.earth>
Eric Culbertson added 2 commits August 5, 2019 09:42
Signed-off-by: Eric Culbertson <eculbertson@astraea.io>
Signed-off-by: Eric Culbertson <eculbertson@astraea.io>
@vpipkt

vpipkt commented Aug 6, 2019

Copy link
Copy Markdown
Member

@emc5ud when you get all your changes like you like, just ping me or re-request review

… NoData

Signed-off-by: Eric Culbertson <eculbertson@astraea.io>
@vpipkt vpipkt added this to the 0.8.0 milestone Aug 7, 2019
@vpipkt
vpipkt merged commit 3426ac6 into locationtech:develop Aug 7, 2019
@vpipkt
vpipkt deleted the feature/nodata-add-doc branch August 7, 2019 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants