-
Notifications
You must be signed in to change notification settings - Fork 40
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
fix(csr): Set type of node_props to float64 #235
Conversation
`dtype` of `node_props` passed from `csr_to_nbgraph()` to `NBGraph` was `float32`. This sets the type to explicitly be `np.float64` which matches the expectation of `csr_spec_float` passed to `numba.experimental.jitclass()`.
Cool, thanks for the investigation @ns-rse! Do you think you could copy over the failing test from TopoStats to make sure we don't get a regression here? |
Sure, I'm working on a different project for remainder of the week and then on annual leave for a week so won't be for a little while but happy to do so. |
Remembered I made a start on the requested tests some time ago so have added what I've done so far. Not managed to work out how to extract/abstract the error seen with TopoStats yet but its on my ToDo list (unfortunately a bit of a way down, sorry). |
I'm gonna try to add the tests, @ns-rse, but I'm actually not so sure about the tests you've added, first because csr_to_nbgraph is a semi-private function (meaning, it's technically publicly accessible but it's not documented anywhere — indeed it doesn't even have a docstring, which is the first thing I checked to see if this behaviour was expected), and second because I'm not 100% I want to "enshrine" this behaviour rather than have it be an implementation detail. Of course, there's an argument to be made that it's good to break APIs on purpose rather than accidentally and the tests would force us to do that, which is fine, but... Maybe add a note saying, "this is the behaviour as of 0.12.1 — it is an accidental design and can be changed". I just find that years later I come to such tests and think, hmmm, was there a good reason why this behaviour is being tested like this...? |
In particular I was actually quite surprised that the second tests, passing in other csr arrays, would fail. |
Ok so the good news is, all it takes to reproduce the error is passing float32 values to Skeleton. So I should be able to write a small test for this PR. I am sad that that doesn't work though — ideally I'd like to find a way to support float32 data, because no one likes having their RAM usage triple unnecessarily. 😂 But I'll make a separate issue for that, since the status quo is that only float64 works. |
Thanks @jni. Really useful to see how the test case was constructed too, will bear that in mind for future tasks/tests. |
dtype
ofnode_props
passed fromcsr_to_nbgraph()
toNBGraph
wasfloat32
. This sets the type to explicitly benp.float64
which matches the expectation ofcsr_spec_float
passed tonumba.experimental.jitclass()
.The tests that were previously failing in TopoStats (see #234 for details) now pass locally. 🙂
Closes #234