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

Wrap Enumerated Datatype #1790

Merged

Conversation

nguyenv
Copy link
Collaborator

@nguyenv nguyenv commented Jun 21, 2023

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #30456: Bind Enumerated Data Type in TileDB-Py.

@nguyenv nguyenv force-pushed the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch from ba9bbd6 to 6eb94ef Compare June 22, 2023 01:54
setup.py Outdated Show resolved Hide resolved
@nguyenv nguyenv force-pushed the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch from 6eb94ef to bb9630b Compare June 22, 2023 01:59
@ihnorton ihnorton self-requested a review June 23, 2023 17:06
@nguyenv nguyenv changed the title Wrap Enumerated Datatype [NOMERGE] Wrap Enumerated Datatype Jun 23, 2023
@nguyenv nguyenv changed the title [NOMERGE] Wrap Enumerated Datatype [NOMERGE UNTIL 2.17] Wrap Enumerated Datatype Jun 23, 2023
@nguyenv
Copy link
Collaborator Author

nguyenv commented Jun 24, 2023

Errors are unrelated to changes in this PR. Fix is in #1793.

@nguyenv nguyenv force-pushed the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch 4 times, most recently from 2d76d32 to 40b81e9 Compare June 27, 2023 14:18
@nguyenv
Copy link
Collaborator Author

nguyenv commented Jun 27, 2023

macOS runners (sans Python 3.7) are failing tiledb/tests/cc/test_cc.py::test_array. It is not reproducible on an EC2 macOS instance. Adding timestamps for writing and reading does not help. I do know how this is related to changes with Enumeration. I'll need to stop looking at this for now to work on adding Enumeration to TileDB-SOMA. Ultimately we do not even use the Pybind11 wrapped lt.Array in TileDB-Py yet; we are still using the Cython wrapped Array so fixing this issue is not highest priority.

@nguyenv
Copy link
Collaborator Author

nguyenv commented Jun 27, 2023

On Windows we do not get to the step of installing TileDB-Py. It is erroring out when building libtiledb.

https://github.com/TileDB-Inc/TileDB-Py/actions/runs/5390955093/jobs/9787083896?pr=1790#step:13:408

D:\a\TileDB-Py\TileDB-Py\build\TileDB-d43f9123\tiledb\..\tiledb/sm/serialization/array_schema.h(129,43): error C3083: 'Enumeration': the symbol to the left of a '::' must be a type (compiling source file D:\a\TileDB-Py\TileDB-Py\build\TileDB-d43f9123\tiledb\sm\c_api\tiledb.cc) [D:\a\TileDB-Py\TileDB-Py\build\TileDB-d43f9123\build\tiledb\tiledb\TILEDB_CORE_OBJECTS.vcxproj] [D:\a\TileDB-Py\TileDB-Py\build\TileDB-d43f9123\build\tiledb.vcxproj

@johnkerl
Copy link
Contributor

[sc-30316]

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #30316: Enumerated data types AKA categoricals AKA factors.

@nguyenv
Copy link
Collaborator Author

nguyenv commented Aug 17, 2023

Errors are unrelated to this PR. There is an ongoing fix in core for dimension labels on dense arrays.

@johnkerl
Copy link
Contributor

johnkerl commented Sep 6, 2023

@nguyenv can you please rebase this on top of today's dev? Or, I have already done so locally and can push my rebase -- if that's OK with you ...

@johnkerl johnkerl mentioned this pull request Sep 6, 2023
@nguyenv
Copy link
Collaborator Author

nguyenv commented Sep 6, 2023

Oh just noticed this comment. Please go ahead and push your changes.

@johnkerl johnkerl force-pushed the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch from 6ee967e to 94c39c7 Compare September 6, 2023 19:08
@johnkerl
Copy link
Contributor

johnkerl commented Sep 6, 2023

@nguyenv I for the most recent CI fail I ran clang-format -i tiledb/query_condition.cc on my laptop & pushed but it doesn't like that. I'm missing some settings I think. I tried reverse-engineering a run-line from the CI job but my laptop doesn't like --clang-format-executable flag ... 🤔

@johnkerl
Copy link
Contributor

johnkerl commented Sep 6, 2023

pip install clang-format==10.0.1 to the rescue -- forget about apt et al. :)

@johnkerl johnkerl force-pushed the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch from 2c9fc27 to 00a3924 Compare September 7, 2023 15:03
@nguyenv nguyenv force-pushed the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch from 0bb218a to b9744e3 Compare September 11, 2023 11:28
@nguyenv nguyenv marked this pull request as ready for review September 11, 2023 14:50
@ihnorton ihnorton changed the title [NOMERGE UNTIL 2.17] Wrap Enumerated Datatype Wrap Enumerated Datatype Sep 14, 2023
@ihnorton ihnorton force-pushed the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch from 940aea6 to a9aadd0 Compare September 14, 2023 16:39
@eddelbuettel
Copy link
Contributor

pip install clang-format==10.0.1 to the rescue -- forget about apt et al. :)

Apparently clang-format-14 (which is one used by core) arrived in Ubuntu 22.04 and has been available in every release since: https://packages.ubuntu.com/search?suite=all&section=all&arch=any&keywords=clang-format-14&searchon=names

clang-format-10, if you really want it, was in 20.04: https://packages.ubuntu.com/search?keywords=clang-format-10&searchon=names&suite=all&section=all

(And I still don't understand why our script leaves it open, just calls the unversioned clang-format and sets us up for needless delta between version. Oh well.)

@ihnorton ihnorton merged commit 498b2b1 into dev Sep 14, 2023
@ihnorton ihnorton deleted the viviannguyen/sc-30456/bind-enumerated-data-type-in-tiledb-py branch September 14, 2023 19:07
@ihnorton
Copy link
Member

Release checks:

In [1]: import tiledb

In [2]: tiledb.version()
Out[2]: (0, 23, 0)

In [3]: tiledb.libtiledb.version()
Out[3]: (2, 17, 0)

In [4]: a = tiledb.open("s3://<...>/0.23.0-check")

In [5]: a[:]
Out[3]:
array([0.59115895, 0.60078051, 0.41469102, 0.57943571, 0.95046928,
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants