Repository navigation
RFC: restrict table._column_mixin to CPython's limited API - #18227
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
table._column_mixin Cython module for compatibility with CPython's limited API
a60206b to
1c693d1
Compare
1c693d1 to
86c6a27
Compare
table._column_mixin Cython module for compatibility with CPython's limited APItable._column_mixin to CPython's limited API
|
@neutrinoceros - now that #18236 is merged, could you rebase and remove the exceptions for astropy/table? (and similarly for the WCS PR) |
eed01a4 to
01fc1a1
Compare
5a6eafb to
310f16b
Compare
mhvk
left a comment
There was a problem hiding this comment.
I missed this one earlier - this looks like a good solution to me, and should impose little or no slowdown.
|
@astrofrog - I have no opinion on the code itself (defer to @mhvk on that), but can you some minimal profiling to confirm that performance is not impacted? |
|
Benchmarks already ran successfully. Do you want something more fine grained ? |
|
For the record, here are the benchmarks we are running: https://github.com/astropy/astropy-benchmarks/blob/main/benchmarks/table.py which benchmark(s) is/are the most relevant to look at here? (as @neutrinoceros you can see the results in the CI job for benchmarks) |
|
I see the benchmarks ran. I downloaded the artifacts but the JSON files there are not immediately useful. Are there tools that help to analyze the data? Looking at the table tests, the closest test to getting a single item from a column is: However, this includes getting the column In any case, I am satisified that the code does not impact item-get performance with this: There was no appreciable difference between main and this branch, so all good! |
310f16b to
db9a0a5
Compare
|
rebased to refresh CI (some required jobs were not triggered). |
|
The Check PR change log job failed to get started. Not sure how to fix that. |
db9a0a5 to
f0275be
Compare
|
There was some Ubuntu server hiccup too. I just rebased it so everything would rerun. Is this good to merge if CI green? Thanks, all! |
|
Benchmark failure unrelated (astropy/astropy-benchmarks#137) but also means we cannot test that in PR right now.
|
Description
Partially address #18163
I wanted to open this quickly to set an example of how many of our compile-time incompatibilities regarding the use of
PyTypeObjectcan be resolved.ping @astrofrog