mypy init #78
mypy init #78
Conversation
|
@aloctavodia I think I have made good progress. I still need to work on the docstrings and the rest of the files. Still, I think this is a good checkpoint to get some feedback as I am touching many places :) Don't feel pressured to review as I will continue working on it. |
Co-authored-by: Osvaldo A Martin <aloctavodia@gmail.com>
|
Test are passing locally but on the ci/cd we are getting ModuleNotFoundError: No module named 'pymc.logprob.joint_logprob'because of pymc-devs/pymc#6599. I updated the path in ccd98b8 |
Great! I did change some small things which should not have any real impact on the code performance but feel free to test without rushing :) |
There was a problem hiding this comment.
The changes introduced a performance drop of around 50% to 65%. The main reason seem to be the change from list to array and then performing operations such as np.append on arrays. Such operations are too slow, the difference is small for a few calls, but for thousands of call, the effect quickly adds up to many seconds.
I have suggested changes, but not sure if mypy is going to complain.
| leaf_node = self.get_node(node_index) | ||
| output[leaf_node.idx_data_points] = leaf_node.value | ||
|
|
||
| if self.idx_leaf_nodes is not None: |
There was a problem hiding this comment.
self.idx_leaf_nodes should not be None when calling _predict, it can only be None after trimming, but then we don't call _predict
There was a problem hiding this comment.
Unfortunately, mypy just cares about the type and does not follow what is happening outside the methods 😞
|
Thank you @aloctavodia ! I will look into your comments in detail! |
Co-authored-by: Osvaldo A Martin <aloctavodia@gmail.com>
Co-authored-by: Osvaldo A Martin <aloctavodia@gmail.com>
Co-authored-by: Osvaldo A Martin <aloctavodia@gmail.com>
Co-authored-by: Osvaldo A Martin <aloctavodia@gmail.com>
Co-authored-by: Osvaldo A Martin <aloctavodia@gmail.com>
|
@aloctavodia I addressed your comments... would you mind running the benchmark to see if we do not have performance degradation with these changes? Thanks :) |
|
According to the benchmark, this may be 5% faster or 5% slower, haha. Joking aside I think this is ready to merge, I am going to wait a little bit in case @fjloyola has some comments to add. |
|
Amazing! Thanks for the feedback! |
|
Thanks @juanitorduz!!! |
First steps towards adding mypy support, see #76