DRAFT: add validation in python for NormalAccessor #328
Conversation
|
This could potentially be quite off base. But just took a stab at it for a draft. This PR was created in response to reference here: #322 (comment) |
|
We should add a mention of the pre-commit setup in DEVELOP.md, but you need to run Your PR is erroring on formatting. |
| super().__init__(*args, **kwargs) | ||
| self.tag(sync=True, **NORMAL_SERIALIZATION) | ||
|
|
||
| def validate( |
There was a problem hiding this comment.
I'd recommend writing some tests first, which will make it easier to sort out the logic in this function. You can find some related tests here.
The purpose of this validation function is to take whatever user input and both validate it and conform to a standard expectation of how we want to store it internally.
To align with deck.gl and other traits, we should allow a list with three scalar values, or a numpy array, or a pyarrow FixedSizeListArray.
But here the first thing you check is value.shape. Which won't exist for a list type. So it'll crash
In [1]: l = [1, 23, 50]
In [2]: l.shape
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[2], line 1
----> 1 l.shape
AttributeError: 'list' object has no attribute 'shape'
Hence why adding tests first might help
| @@ -64,6 +64,14 @@ Anywidget and its dependency ipywidgets handles the serialization from Python in | |||
|
|
|||
| Push a new tag to the main branch of the format `v*`. A new version will be published to PyPI automatically. | |||
|
|
|||
| ## Contribute | |||
There was a problem hiding this comment.
This whole doc is geared towards contributors; maybe put this section above ## Publishing and name it ## Linting?
| if not all(isinstance(item, float) for item in value): | ||
| self.error("All elements of Normal array must be floating point type") | ||
|
|
||
| return pa.FixedSizeListArray.from_arrays(value, fixed_list_size) |
There was a problem hiding this comment.
This won't work...
The conceptual difference to understand is between a scalar which will be used as the value for all rows and a column, where a different value per row can be passed.
It's a bit confusing because a normal value has three elements, but an input value looking like [0, 0, 1] is still a scalar because that value applies to all rows.
To pass a scalar into @geoarrow/deck.gl-layers, we want to pass a JSON-like object, like [0, 0, 1], which it will interpret as a single scalar. Passing a pyarrow Array will cause @geoarrow/deck.gl-layers to interpret the input as a column instead. But then it will error because this "column" has only one row, whereas the table has multiple rows.
Instead you should be returning the input list or tuple
Line 230 in 2ba95c9
| if not np.any(value == 1): | ||
| self.error( | ||
| obj, value, info="One of the normal vector elements must be 1." | ||
| ) |
There was a problem hiding this comment.
This is a great idea for a validation step, but I don't think it's correct. There's no good documentation in deck.gl of the exact format expected. Most likely, these normal vectors are expected to be "normalized" which means that their length is expected to be 1. I.e. x^2 + y^2 + z^2 = 1. This means that almost never will one of the elements themselves be 1.
In any case I'd tend to think this is something we shouldn't validate, because deck.gl might be lenient and allow non-normalized vectors.
|
Hey @kylebarron I will start a new branch that's better synced with |
Adds NormalAccessor and draft tests Respond to comments on #328 and move recent work here. Branch #328 will be deleted --------- Co-authored-by: Kyle Barron <[email protected]>
Draft PR to add NormalAccessor validation to Python experimental traits in
lonboard.I had some unrelated git history issues as I renamed the local repository to help stay organized, hence the branch name.