Skip to content
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

Rerun and clean notebooks with Python 3.9 #1947

Merged
merged 10 commits into from
Jun 30, 2023
Merged

Conversation

miguelgfierro
Copy link
Collaborator

Description

Related Issues

References

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging branch and not to main branch.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

We don't get the progress bar any more?

Respond and view the context here.

Copy link
Collaborator

Any idea why these value counts changed? The data set should be the same, shouldn't it? So value counts should remain the same too.

Respond and view the context here.

Copy link
Collaborator

This looks fishy, the logloss should be the same, since ground truth and predictions have not changed.

Respond and view the context here.

Copy link
Collaborator

Same here.

Respond and view the context here.

Copy link
Collaborator Author

fixed in the next commit

Respond and view the context here.

Copy link
Collaborator Author

fixed by reruning

Respond and view the context here.

Copy link
Collaborator Author

this is weird, I looked at the history of this file and 4.1 was obtained the first time it was computed years ago. I have rerun the notebook with python 3.9 and sklearn 1.0.2 and I get 5.25. I have also tried python 3.8 and sklearn 0.24.2 and I got the same result. I don't know, maybe the first time was wrong?

Respond and view the context here.

Copy link
Collaborator Author

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anargyri I have reviewed the comments and fixed what you mentioned. I couldn't find a fix for the logloss, see my comments

@miguelgfierro
Copy link
Collaborator Author

I don't understand very well why the SAR notebooks are not passing. They were passing beofre, we are getting the same error in the other PRs, but for example, in this one, we haven't touched that file.
It works in local

Copy link
Collaborator

Ok, it may have been a bad output that was somehow left there.

Respond and view the context here.

@anargyri
Copy link
Collaborator

anargyri commented Jun 28, 2023

I don't understand very well why the SAR notebooks are not passing. They were passing beofre, we are getting the same error in the other PRs, but for example, in this one, we haven't touched that file. It works in local

It looks like the sparse matrix product causes this error https://github.com/microsoft/recommenders/actions/runs/5392089967/jobs/9789938422?pr=1947#step:3:2468
Maybe something changed in a more recent version of scipy?
Or maybe the sparse matrices are not computed correctly? You may need to check what the matrices are. Did you try this test on a VM?

@anargyri
Copy link
Collaborator

I don't understand very well why the SAR notebooks are not passing. They were passing beofre, we are getting the same error in the other PRs, but for example, in this one, we haven't touched that file. It works in local

It looks like the sparse matrix product causes this error https://github.com/microsoft/recommenders/actions/runs/5392089967/jobs/9789938422?pr=1947#step:3:2468 Maybe something changed in a more recent version of scipy? Or maybe the sparse matrices are not computed correctly? You may need to check what the matrices are. Did you try this test on a VM?

I tried it on a VM and it fails with this error.

@miguelgfierro
Copy link
Collaborator Author

That's interesting, in my local computer it passes.
These are the versions I have:

System version: 3.9.16 (main, May 15 2023, 23:46:34) 
[GCC 11.2.0]
Pandas version: 1.5.3
NumPy version: 1.24.3
Scipy version: 1.10.1

Can you check the ones where it breaks @anargyri?

@anargyri
Copy link
Collaborator

That's interesting, in my local computer it passes. These are the versions I have:

System version: 3.9.16 (main, May 15 2023, 23:46:34) 
[GCC 11.2.0]
Pandas version: 1.5.3
NumPy version: 1.24.3
Scipy version: 1.10.1

Can you check the ones where it breaks @anargyri?

Python and pandas versions are the same, numpy is 1.24.4, scipy is 1.11.0 , gcc is 9.4.0.

@anargyri
Copy link
Collaborator

anargyri commented Jun 29, 2023

That's interesting, in my local computer it passes. These are the versions I have:

System version: 3.9.16 (main, May 15 2023, 23:46:34) 
[GCC 11.2.0]
Pandas version: 1.5.3
NumPy version: 1.24.3
Scipy version: 1.10.1

Can you check the ones where it breaks @anargyri?

Python and pandas versions are the same, numpy is 1.24.4, scipy is 1.11.0 , gcc is 9.4.0.

It could be related to this change https://scipy.github.io/devdocs/reference/sparse.html#module-scipy.sparse https://scipy.github.io/devdocs/release/1.11.0-notes.html

@miguelgfierro
Copy link
Collaborator Author

@anargyri the issue with scipy is fixed, I'm reruning the tests, hopefuly they pass. Please review.

@miguelgfierro miguelgfierro merged commit d284e5e into staging Jun 30, 2023
26 checks passed
@miguelgfierro miguelgfierro deleted the miguel/rerun_cpu_deeps branch June 30, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants