-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Handle the character array dim name #2896
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
Changes from 7 commits
422ce01
70799e2
6cf937f
7352e50
9bdc94f
5c57885
f5ffd48
7a1753f
609e475
cda3933
6070f7c
768dc31
b190417
fc13ea2
1de5b3f
1c1a543
eb4b055
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,35 @@ def test_CharacterArrayCoder_encode(data): | |
assert_identical(actual, expected) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
'original', | ||
[ | ||
Variable(('x',), [b'ab', b'cdef']), | ||
Variable(('x',), [b'ab', b'cdef'], encoding={'char_dim_name': 'foo'}) | ||
] | ||
) | ||
def test_CharacterArrayCoder_char_dim_name(original): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is better and getting warmer. The test improved the underlying code here. But I still dont see how to eliminate this logic. Putting an Thanks, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The clean way to do this would be to add a second parametrized argument for the expected value, e.g.,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is the first case of 2-variable usage in that test file... thanks for pointing it out. |
||
coder = strings.CharacterArrayCoder() | ||
|
||
encoded = coder.encode(original) | ||
roundtripped = coder.decode(encoded) | ||
assert encoded.encoding == roundtripped.encoding | ||
assert encoded.dims[-1] == encoded.encoding['char_dim_name'] | ||
assert roundtripped.dims[-1] == original.dims[-1] | ||
|
||
# To compare with the original requires logic since encoding either preserves | ||
# encoding['char_dim_name'] or it creates it from scratch. Could hardcode to get | ||
# around the logic but then the test is brittle to change/addition in the | ||
# parametrized data used and similarly confusing in its arbitrariness. | ||
if 'char_dim_name' in original.encoding.keys(): | ||
expected_char_dim_name = original.encoding['char_dim_name'] | ||
else: | ||
expected_char_dim_name = 'string%s' % encoded.data.shape[-1] | ||
|
||
assert encoded.encoding['char_dim_name'] == expected_char_dim_name | ||
assert roundtripped.encoding['char_dim_name'] == expected_char_dim_name | ||
|
||
|
||
def test_StackedBytesArray(): | ||
array = np.array([[b'a', b'b', b'c'], [b'd', b'e', b'f']], dtype='S') | ||
actual = strings.StackedBytesArray(array) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line is causing a set of failures. I think this line is desirable. I could be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 111, to be clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention we have (which is apparently enforced by tests) is that when you use an encoding you remove it from the
encoding
dict. That way we catch cases where you have a typo, e.g.,char_dimension_name
instead ofchar_dim_name
.So this section should instead probably be something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great reason, this solves all the errors I was seeing locally. THanks