-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
sage.misc.prandom misses several functions added in Python 3 #34973
base: develop
Are you sure you want to change the base?
sage.misc.prandom misses several functions added in Python 3 #34973
Conversation
Hello everyone, I have created this pull request in view of changes committed by me in the open issue #32439, Please review this pull request and tell if there are changes that needs to be done on my side. |
Codecov ReportBase: 88.58% // Head: 88.59% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #34973 +/- ##
===========================================
+ Coverage 88.58% 88.59% +0.01%
===========================================
Files 2140 2140
Lines 397273 397279 +6
===========================================
+ Hits 351933 351988 +55
+ Misses 45340 45291 -49
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@kwankyu Sir, can you review this pull request, or tell someone to for the review. |
There are various test failures - https://github.com/sagemath/sage/actions/runs/4132992607/jobs/7142356394#step:11:6044 |
Don't I have to add more functions or add class, as in that case I would add that code first and make my code clean. |
Yes, that's why I set it to "needs work". There's no need for a PR to fix everything in an issue at the same time; but those parts that you do fix need to pass their tests. I recommend that you expand the PR description to say more precisely what this PR does. One difficulty here is to achieve compatibility with different versions. Some of the functions mentioned in #32439 are only added in Python 3.9 (or later?), but Sage still supports Python >= 3.8. |
I think, I would correct my code first to make sure tests passing. And then describe my PRs purpose as completing the whole issue |
@mkoeppe sir, I was trying to fix failing tests, I recognized that none of them was caught in my machine, and all tests are working fine. Can you tell me how should I proceed ? |
As I said:
So you may want to test using Python 3.8 on your machine. |
@mkoeppe, indeed the failing tests are due to fact that those features appeared in python 3.9 and not in python 3.8. |
@mkoeppe , can you tell me what should I do further ? |
To move this PR forward, you can just cut out these functions that were only added in Python 3.9. In a follow-up PR, to be merged when we drop Python 3.8, these additional functions can then be added. |
<!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to multiply two integers" --> ### 📚 Description In preparation for important package upgrades, we remove support for Python v3.8. (Almost) all occurrences of "Python 3.8" are removed from the codebase. There are still some comments left that essentially say that in newer versions things can be simplified, but I didn't touch those and leave them for follow-up PRs. This will help with: - PRs that require Python 3.9 or above, such as sagemath#34973 and sagemath#35188. - Upgrades of Python packages that have dropped support for Python 3.8 because they follow NEP 29 and have already released a new major version that drops support according to the NEP 29 schedule. - NetworkX 3.2 (expected 2023-??; version 3.1 was released 2023-04, see sagemath#35671) - sagemath#34816 - sagemath#35703 - See also sagemath#32074. Test run: https://github.com/tobiasdiez/sage/actions/runs/4586981626 <!-- Describe your changes here in detail. --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#35404 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Matthias Köppe
<!-- Please provide a concise, informative and self-explanatory title. --> <!-- Don't put issue numbers in the title. Put it in the Description below. --> <!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to multiply two integers" --> ### 📚 Description In preparation for important package upgrades, we remove support for Python v3.8. (Almost) all occurrences of "Python 3.8" are removed from the codebase. There are still some comments left that essentially say that in newer versions things can be simplified, but I didn't touch those and leave them for follow-up PRs. This will help with: - PRs that require Python 3.9 or above, such as sagemath#34973 and sagemath#35188. - Upgrades of Python packages that have dropped support for Python 3.8 because they follow NEP 29 and have already released a new major version that drops support according to the NEP 29 schedule. - NetworkX 3.2 (expected 2023-??; version 3.1 was released 2023-04, see sagemath#35671) - sagemath#34816 - sagemath#35703 - See also sagemath#32074. Test run: https://github.com/tobiasdiez/sage/actions/runs/4586981626 <!-- Describe your changes here in detail. --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x ]`. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#35404 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Matthias Köppe
Documentation preview for this PR (built with commit 4d368e5; changes) is ready! 🎉 |
|
This is my pull request originally created on trac, which I thought pushing on github.
Fixes #32439