Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NormalAccessor #376

Merged
merged 7 commits into from
Feb 29, 2024
Merged

NormalAccessor #376

merged 7 commits into from
Feb 29, 2024

Conversation

naomatheus
Copy link
Contributor

Adds NormalAccessor and draft tests
Respond to comments on #328 and move recent work here. Branch #328 will be deleted

lonboard/traits.py Show resolved Hide resolved
lonboard/traits.py Outdated Show resolved Hide resolved
lonboard/traits.py Outdated Show resolved Hide resolved
"""Warning: Numpy array should be floating point type.
Converting to float32 point pyarrow array"""
)
value = value.astype(np.float(pa.list_(pa.float32(), 3)))
Copy link
Member

Choose a reason for hiding this comment

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

This will fail; you want value.astype(np.float32). You can't pass a pyarrow type into a numpy function like this.

lonboard/traits.py Outdated Show resolved Hide resolved
with pytest.raises(TraitError):
NormalAccessorWidget()

with pytest.raises(TraitError, match="expected 3 values if passed a tuple or list"):
Copy link
Member

Choose a reason for hiding this comment

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

The match here is tested against the error string, so if you're copying tests from above you might need to adjust the expected error string, or use the same error message in your code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I will update these to match

tests/test_traits.py Outdated Show resolved Hide resolved
# expected value is a numpy ndarray with two dimensions
# And size of the second dimension must be 3 i.e. `(N,3)`
with pytest.raises(TraitError, match="expected shape (N,3)"):
NormalAccessorWidget(value=np.array([1, 2], 3))
Copy link
Member

Choose a reason for hiding this comment

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

All of these numpy calls will fail.

To construct a 2d numpy array you can do either

np.array([[1, 2], [3, 4]])

which gives a 2x2 numpy array or

np.array([1, 2, 3, 4, 5, 6]).reshape(-1, 2)

which first creates a 1-dimensional array and then rearranges into a 2-d array, in this case of shape (3, 2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I'll bring this as a general testing question because I thought that we were intentionally creating fail conditions and ensuring that the NormalAcc fails on these objects (like where the shape is just incorrect).

I see. Will continue updating the proper tests

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I mean that we want to test that passing a valid numpy array into NormalAccessorWidget fails. If the numpy creation fails before passing into our code, we haven't tested our own code yet.

naomatheus and others added 5 commits February 26, 2024 13:36
Co-authored-by: Kyle Barron <kyle@developmentseed.org>
Co-authored-by: Kyle Barron <kyle@developmentseed.org>
Co-authored-by: Kyle Barron <kyle@developmentseed.org>
@kylebarron kylebarron marked this pull request as ready for review February 29, 2024 21:06
@kylebarron kylebarron changed the title Draft - NormalAccessor NormalAccessor Feb 29, 2024
@kylebarron
Copy link
Member

Thanks! I cleaned this up and updated the tests

@kylebarron kylebarron merged commit 27e3ef9 into main Feb 29, 2024
5 checks passed
@kylebarron kylebarron deleted the matt/normalaccessor branch February 29, 2024 21:07
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.

None yet

2 participants