-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve types handling #82
Conversation
e578fff
to
79e71b9
Compare
…ter (memory heavy)
f5ba059
to
06aab04
Compare
Great work!! Thanks for your work |
Thanks for your time @romainx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Thanks a lot for this work.
I have reviewed the code and here are my remarks.
As I said, I will perform the changes after the merge.
-
The unit test fails due to the cache used. That seems to not be cleared in some cases. It's better to call the clear cache at the beginning of
describe
. I'm not a big fan of caching in a global variable. But we can keep it as is. It's simple to remove if it causes problem. -
Using
type
as a variable name like inget_vartype
is not a good practice since it can hide the build-intype
function. We should usevartype
instead -
I will take into account new keyword arguments in the
ProfileReport
docstring. It's important since the help description will appear in Jupyter notebooks.
I've not tried the unsupported type -> I will try to build a test case for it.
Hey, thanks for your observations!
I'm also not a fan of cache in global variables, indeed this part needs a good refactoring. I tried to simplify this first version before doing a refactoring. I evaluated some cache strategies like the lru-cache (built-in python 3) but it doesn't work since the function parameter is not a base type. (I also tried two other packages, but most of then relies on the parameter type) Interesting is that the cache didn't fail in the tests here, not sure why.
I agree.
Ups, my mistake.
I added some test cases for the unsupported types, not sure if you mean to add a test case for that or add more tests. Best |
OK, I'm responding each issue with the related fix/improvement done by this PR. Best |
Done. I explained in each issue what was solved and requested the issue to be closed. In some issues I requested to you the openning of a new issue for a specific subject discussed in that issue (to separate the problem apart of new suggestions) Best |
Improve types handling
The goal of this PR is to stabilize the handling of different types and to improve tests. The changes are:
This should solve #76 #77 #70 #44 #29 #66
Please review my changes and if agreed merge it.
Best