Conversation
| offset = ( | ||
| x.ctypes.get_as_parameter().value | ||
| - x.base.ctypes.get_as_parameter().value | ||
| x.ctypes._as_parameter_.value - x.base.ctypes._as_parameter_.value |
There was a problem hiding this comment.
Initially I was concerned to see us using a private _as_parameter_ attribute, however after looking a little closer this is what NumPy recommends so this seems okay. Just posting a quick comment here in case other had a similar thought
There was a problem hiding this comment.
yeah I had the same reaction, but the warning specifically recommended using the private attribute 🤷
|
Woohoo! The upstream tests are passing and it looks like the failing test is just a flaky one. Good with you if I merge @jrbourbeau? |
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks as always for handling this @jsignell! Overall this looks good, just left a couple of small comments
| @pytest.mark.filterwarnings( | ||
| "ignore:Dropping of nuisance columns:FutureWarning" | ||
| ) # https://github.com/dask/dask/issues/7714 |
There was a problem hiding this comment.
Just to confirm, we can remove this warning filter once we've implemented numeric_only= as mentioned over in #7714?
There was a problem hiding this comment.
Yeah exactly. So filter the warning for now, and hopefully implement soon. Following the pattern in #7100
| { | ||
| "A": np.random.choice([1, 2, np.nan], 100), | ||
| "B": np.random.choice(["a", "b", np.nan], 100), | ||
| "B": np.random.choice(["a", "b", "nan"], 100), |
There was a problem hiding this comment.
This promotion was happening silently before, but numpy doesn't tolerate that anymore.
| _numpy_117 = LooseVersion(np.__version__) >= "1.17.0" | ||
| _numpy_118 = LooseVersion(np.__version__) >= "1.18.0" | ||
| _numpy_120 = LooseVersion(np.__version__) >= "1.20.0" | ||
| _numpy_121 = LooseVersion(np.__version__) >= "1.21.0" |
There was a problem hiding this comment.
This is only used in a comment so far, but it really helps to have it for greppability
|
Thanks for the review @jrbourbeau! Good to merge? |
Addressing some of #7100 and #7710