Conversation
obi1kenobi
left a comment
obi1kenobi
left a comment
There was a problem hiding this comment.
Yup, purely defensive — agreed that it's probably safe to merge if tests pass.
Pointing out two minor things for you to consider. I don't have strong feelings one way or the other, just wanted to point them out to make sure it's an intentional decision whichever way it goes.
| # fake 8-school output | ||
| obj = {} | ||
| for key, shape in {"mu": [], "tau": [], "eta": [8], "theta": [8]}.items(): | ||
| shapes: Mapping[str, list] = {"mu": [], "tau": [], "eta": [8], "theta": [8]} |
There was a problem hiding this comment.
Consider something more specific than list so it captures the types within the list? I'm not sure what np.random.randn requires the type of shape to be, exactly.
There was a problem hiding this comment.
i was actually surprised this popped up -- this is a literal dict used once, so List[int] (or something...) seems like it should have been deduced.
|
The fix is great but can we also get a new release? |
3 participants
Fixes #1947. I also had some trouble installing some
requirements-external.txt, but I didn't include those in the change so far.I'll tag @obi1kenobi who originally added this pin, I think it was just defensive against the next major version, but if the mypy checks pass, I think we can merge.