-
Notifications
You must be signed in to change notification settings - Fork 346
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
[MRG] Fix numpy versions problems #411
[MRG] Fix numpy versions problems #411
Conversation
Hello, there seems to be a problem related to numpy versions into tslearn's main branch. Part of the error is related to the lines: import numpy as np in the file: Here is the error message: init.pxd:942: in numpy.import_array This error message looks to be related to different versions of numpy being installed: The solution of this error message looks to be to upgrade numpy: |
At the beginning I was surprised by the import lines: import numpy as np but it seems to be correct: |
We should use numpy version <= 1.22 as I have the following error message: |
Now we have the following error message when running the tests on Linux with Python 3.7:
Python 3.7 requires NumPy version <= 1.21.6 |
Now we have the following error message: tslearn/metrics/cysax.pyx:1: in init tslearn.metrics.cysax which also seems to be related to numpy versions. |
Now there is only one test being run: docs/readthedocs.org:tslearn (successful) |
Hello @rtavenar and @GillesVandewiele, |
Hi @YannCabanes I have been doing a bit of unsuccessful digging into these issues myself. My 2 cents I can already give straight away (I will look further in depth into this later) is that these |
Now, I have the following error message: I have previously tried to write: python -m pip install numpy==1.22 |
I am not sure...this is based on what I read in a scipy PR: scipy/scipy#14813
So, would you mind trying this?
Can you also add the same thing for btw, And, then see if this issue can be resolved or if we get a new error or not. |
Here are the execution times of the functions previously coded in Cython.
Small time series Functions of file cycc.py TEST_NORMALIZED_CC TEST_CDIST_NORMALIZED_CC TEST_Y_SHIFTED_SBD_VEC Functions of file cysax.py TEST_INV_TRANSFORM_PAA TEST_CYDIST_SAX TEST_INV_TRANSFORM_SAX TEST_CYSLOPES TEST_CYDIST_1D_SAX TEST_INV_TRANSFORM_1D_SAX Functions of file soft_dtw_fast.py TEST_SOFTMIN3 TEST_SOFT_DTW TEST_SOFT_DTW_GRAD TEST_JACOBIAN_PRODUCT_SQ_EUC Large time series Functions of file cycc.py TEST_NORMALIZED_CC TEST_CDIST_NORMALIZED_CC TEST_Y_SHIFTED_SBD_VEC Functions of file cysax.py TEST_INV_TRANSFORM_PAA TEST_CYDIST_SAX TEST_INV_TRANSFORM_SAX TEST_CYSLOPES TEST_CYDIST_1D_SAX TEST_INV_TRANSFORM_1D_SAX Functions of file soft_dtw_fast.py TEST_SOFTMIN3 TEST_SOFT_DTW TEST_SOFT_DTW_GRAD TEST_JACOBIAN_PRODUCT_SQ_EUC |
tslearn/metrics/softdtw_variants.py
Outdated
@@ -308,12 +310,14 @@ def gamma_soft_dtw(dataset, n_samples=100, random_state=None): | |||
---------- | |||
.. [1] M. Cuturi, "Fast global alignment kernels," ICML 2011. | |||
""" | |||
return 2. * sigma_gak(dataset=dataset, |
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.
Here I find the original version easier to read...
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 @rtavenar,
Are you talking about the code aesthetics of the file softdtw_variants.py?
I think that I only did a few useful modifications in this file, most of the "aesthetics" modifications have been performed by the "black" command. The command: "black + filename" in the terminal automatically corrects Python codes aesthetics to make sure that it corresponds to PEP8 conventions. It is very practical to make sure that the PEP8 conventions are respected, but sometimes it might not be the "aesthetics" version prefered by the users.
Would you like make to come back to the previous presentation?
And more generally, want do you think of using the "black" command?
Personally, I find "black" very convenient, but I wouldn't want it to come at the expense of aesthetics or code clarity.
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.
I just think that, in this case, black made things worse instead of better-looking.
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.
Ok, I will restore the previous formulation.
I have removed the signatures (input and output types) in jit decorators in the last commit. Information about signatures in jit decorators can be found at:
|
The last continuous integration test gave the following error message: =================================== FAILURES =================================== name = 'LearningShapelets'
tslearn/tests/test_estimators.py:215: tslearn/tests/test_estimators.py:197: in check_estimator x = array([[3.7043095e-03],
E AssertionError: /opt/hostedtoolcache/Python/3.9.14/x64/lib/python3.9/site-packages/sklearn/utils/_testing.py:418: AssertionError |
There is still the same error message. |
The tests are failing with Linux, they pass with MacOS and Windows. |
There is still the same failing tests with the signatures in the jit decorators. |
adac6d7
to
eb111e1
Compare
Hello @rtavenar, I have resolved all conversations except for one conversation with you about codes esthetics. Also, could you look at the failing tests and give me your opinion please? Once these questions are answered, I think that we will be able to merge this PR. |
Hi, It is unclear to me why some tests fail on Linux. These tests are checking that LearningShapelets model behave the same when used in a pipeline or alone, and to do so a seed is applied. In the model's fit, this seed is used to set the seed for both numpy and TF. I will try to find out why, but if you have any idea, it would be helpful. |
The discussion about codes esthetics has been resolved by the last commit. |
On my local computer, the following message is related to the tests of the class
|
It seems that the continuous integration failing test on Linux is not related to the Python files cycc.py, cysax.py, soft_dtw.py, soft_dtw_fast.py and softdtw_variants.py which are the only Python files related to this PR. |
106d53b
to
1975b0f
Compare
No description provided.