Move deprecation policy to the documentation, and update it #22900
Conversation
There is a little bit of duplication here with the stuff at the top of the page which will need to be cleaned up a bit.
This also includes some changes to simplify the constructor to SymPyDeprecationWarning which I have not made yet.
…recations document
asmeurer
added
the
Needs Decision
|
✅ Hi, I am the SymPy bot (v163). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
🟠Hi, I am the SymPy bot (v163). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it. This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75. The following commits add new files:
If these files were added/deleted on purpose, you can ignore this message. |
|
|
||
| - The existing API is confusing. | ||
| - There is unnecessary redundancy in the API. | ||
| - The existing API limits what is possible. |
There was a problem hiding this comment.
Looking through the list of currently open issues tagged with deprecation removal it seems that the most common reason is just because a change is being made to something that seems private but it isn't clear whether anyone downstream might be using. The problem is that the distinction between public and private is not clearly delineated anywhere so we end up deprecating things that probably no one cares about.
There was a problem hiding this comment.
Another reason would be because something is fundamentally broken and cannot be made to work sensibly without incompatible changes.
| are required to make breaking changes). | ||
|
|
||
| And broadly, a non-exhaustive list of some things that don't consistent public | ||
| API, and therefore don't require deprecations to change, include |
| to compute previously. | ||
| - The name of a submodule for functions is included in the top-level | ||
| `sympy/__init__.py`. Such functions should be imported directly, like `from | ||
| sympy import <name>`. |
There was a problem hiding this comment.
It's not clear to me what this point means.
There was a problem hiding this comment.
I'll make it clearer. It means something like the name of the submodule sympy.functions.elementary.exponential isn't public API if we wanted to move the files around, because all the public names in that should be imported from the top-level.
It would be better if this was just a function like: I think that a constructor that emits a warning is confusing. |
|
I think the general principles here are all good. |
I don't think there is one. You might be thinking of #22105 |
That's actually a typo on my part. The constructor doesn't emit a warning. There should be a |
|
|
||
| ## When to deprecate and when not to | ||
|
|
||
| Sometimes the API of SymPy must change in an incompatible way. We try to avoid |
There was a problem hiding this comment.
Suggested change:
"We should, in general, avoid this,"
Below it is emphasized in multiple places why changing API is not good, so I think opening with a bit stronger statement is best.
There was a problem hiding this comment.
I did have something written about this but it was lower down. I've moved it to the top of the file and given it its own heading.
| ## When to deprecate and when not to | ||
|
|
||
| Sometimes the API of SymPy must change in an incompatible way. We try to avoid | ||
| this, because changing API breaks people's code, but it is often deemed worthy |
There was a problem hiding this comment.
but it can be deemed worthy to do so in some circumstances.
There was a problem hiding this comment.
I think our default position should be to discourage API changes unless the benefit largely outweighs the cost. Of course, whether something largely outweighs the cost is subjective because we aren't measuring any information to make it objective. But I think the language in this document should give the overall impression that you should make a very strong effort in avoiding API changes at all and make a very strong effort to design APIs that have less likelihood of needing to be changed. Opening the document with language that sets a strong tone early is good. If our language is weak at the opening about this, then the actual actions of developers will be even weaker (maybe too much API changes tolerated).
| top-level `sympy/__init__.py`, since users must use the fully qualified | ||
| module name to import them (note: many such functions are not included | ||
| because they aren't actually public API at all. For these, no deprecations | ||
| are required to make breaking changes). |
There was a problem hiding this comment.
I don't really agree with this one (edit: or maybe I don't understand it). sympy.physics.vector.frame.ReferenceFrame isn't importable like from sympy import ReferenceFrame, but it is certainly public API. Does this bullet say that only names imported by from sympy import * are public?
There was a problem hiding this comment.
There needs to be a more nuanced definition here. For most of the codebase it is true that public API is exposed at top-level like from sympy import X but there are major exceptions like physics, stats, combinatorics...
There was a problem hiding this comment.
I think you aren't understanding it. What I am trying to write here agrees with you.
The basic point is this: if users have to write the module name to access a function (because it can't be imported from the top-level from sympy import X), then the module name itself is part of the public API. This is because if we renamed the module, it would break user code.
OTOH if a function is in the top-level, then users can write from sympy import X instead of from sympy.submodule.submodule import X, so if we rename submodule it won't break user code.
Or at least that's the ideal situation. It doesn't consider users who may be importing top-level names directly from submodules anyway. I've actually seen quite a bit of user code that does things like from sympy.functions import exp instead of from sympy import exp. The fact that we intentionally do this in library code doesn't help here. So in practice, if we ever did want to rename modules or move functions around, we might want to do a deprecation anyway. Maintaining backwards compatibility here is quite trivial (see for instance the deprecation for sympy.printing.ccode, which was done for technical reasons).
I think a more codified public/private API policy should probably include things like underscores for names that aren't supposed to be public. So e.g., sympy.functions would become sympy._functions and all user code should import names from there from the top-level. But this requires a lot of work to actually do, so it's quite out of scope for this PR. For now, I just want to give some general guidelines.
I'll reword these two bullet points to make them clearer.
There was a problem hiding this comment.
I guess I'll just remove the non-public half of this. The problem is that sometimes names are in a file that aren't included in __init__.py but are still public API. The most common example of this is base classes, which we want people to be able to subclass. To really distinguish this requires a lot more work.
The cleanest thing I can say for now is that submodule renames should be deprecated (which as I noted, is not difficult). Even if a function is just moved around, it's very easy to keep an import of it at the top of the file so that direct imports from the old module still work.
There was a problem hiding this comment.
See what you think of it now.
Also it really needs to be made clear here that these lists here are just guidelines. We don't have a real public/private API policy yet. There are always exceptions to the rules.
| - The name of a submodule for functions is included in the top-level | ||
| `sympy/__init__.py`. Such functions should be imported directly, like `from | ||
| sympy import <name>`. | ||
| - Positional argument names. |
There was a problem hiding this comment.
Technically Python permits:
def f(a, b, c=None):
return a, b, c
r = f(a=12, b=13, c=14)So users can rely on positional names. I don't think it is widely used, but code would break if you changed a and b to something else in the function definition. It isn't too hard to deprecate this either.
There was a problem hiding this comment.
We could try to be more strict about this by using positional-only arguments and keyword-only arguments everywhere. I personally like the idea of using more keyword-only arguments, which helps avoid backwards compatibility breaks (#21803). I'm less a fan of forcing positional-only, but technically speaking, it is a way to enforce that the argument name is not part of the API (it also requires Python 3.9 to do syntactically, but it won't be too long before we drop <=3.8).
But even so, if you take something like solve, which calls its first argument f, I don't think users should be relying on that argument being called f. We should be able to rename it to something more readable like expr or eq. There's a bunch of other places where the argument is just called x or expr or something like that. I don't think users should really be using keyword arguments when they call these functions.
It's also my experience that the reverse is more common anyway. Have you seen real examples of users using keyword arguments where they aren't required? Personally I've seen users call things positionally that probably should be keyword-only, but not the other way around.
|
|
||
| ```py | ||
| CoordSystem(name, patch, symbols('x y', real=True) | ||
| ``` |
| include). | ||
|
|
||
| If you want to make an equality of indefinite integrals, use `Eq(integrate(a, x), | ||
| integrate(b, x))` explicitly. |
There was a problem hiding this comment.
Maybe also worth mentioning that if you have an equality already then you can do
Eq(integrate(eq.lhs, x), integrate(eq.rhs, x))
| ``` | ||
|
|
||
| The difference is that `s.is_empty` may return `None` if it is unknown if the | ||
| set it empty. |
| @@ -28,7 +31,6 @@ | |||
|
|
|||
| if USE_PYTEST: | |||
| raises = pytest.raises | |||
| warns = pytest.warns | |||
There was a problem hiding this comment.
Why is this changed?
There was a problem hiding this comment.
See the updated PR description. I've updated warns to do things that the pytest version does not do, so I didn't think it would be a good idea to continue to allow the pytest one to be used when the tests are run in pytest. Specifically:
warnschecks the stacklevel to see if it is set to a level so that the warning shows the user code (can be disabled withwarns(test_stacklevel=False)). A correctly set stacklevel helps users actually see what code of theirs is triggering the warning.- For
SymPyDeprecationWarning, it checks that the newactive_deprecations_targetflag actually corresponds to a real cross-reference target in the active-deprecations.md document (otherwise the URL printed in the warning message won't work). warnsno longer ignores unrelated warnings. This is something that was lamented in several of thetest_pytest.pytests, which I have updated. There was one instance of deprecated code being used in SymPy that was being silenced by this (I will open an issue about it later, but it'splot_implicit, and I had to XFAIL it for now).
| means API in question is not yet read to be deprecated. The deprecation | ||
| warning should inform users of a way to change their code so that it works in | ||
| the same version of SymPy, as well as all future versions (see | ||
| [below](deprecation-documentation)). |
There was a problem hiding this comment.
Really it should be possible for users to change their code so that it works in both past and future versions of sympy. In some cases it is possible because the "correct" way of using a particular SymPy feature has not changed and only an "incorrect" way is being deprecated (example: importing something from a submodule rather than using from sympy import X). In some cases it isn't possible because the "correct" way doesn't exist in previous sympy versions. Either way there should be at least an attempt to explain how cross-version compatibility can be achieved for any downstream library authors.
| Here are examples corresponding to the (imaginary) examples above: | ||
|
|
||
| ```md | ||
| (simplify-this-deprecation)= | ||
| ### `simplify_this()` | ||
|
|
||
| The `sympy.simplify.simplify_this()` function is deprecated. It has been | ||
| replaced with the {func}`~.simplify` function. Code using `simplify_this()` | ||
| can be fixed by replacing `simplfiy_this(expr)` with `simplify(expr)`. The | ||
| behavior of the two functions is otherwise identical. | ||
|
|
||
| This change was made because `simplify` is a much more Pythonic name than | ||
| `simplify_this`. | ||
| ``` | ||
|
|
||
| `````md | ||
| (is-this-zero-y-deprecation)= | ||
| ### `is_this_zero()` second argument | ||
| The second argument to {func}`~.is_this_zero()` is deprecated. Previously | ||
| `is_this_zero(x, y)` would check if x = y. However, this was removed because | ||
| it is trivially equivalent to `is_this_zero(x - y)`. Furthermore, allowing | ||
| to check $x=y$ in addition to just $x=0$ is is confusing given the function | ||
| is named "is this zero". | ||
|
|
||
| In particular, replace | ||
|
|
||
| ```py | ||
| is_this_zero(expr1, expr2) | ||
| ``` | ||
|
|
||
| with | ||
|
|
||
| ```py | ||
| is_this_zero(expr1 - expr2) | ||
| ``` | ||
|
|
||
| ````` | ||
|
|
There was a problem hiding this comment.
This page has a lot of discussion about when something should be deprecated. I think that it would be good if the examples used actually match that rationale i.e. neither of these is a good example. In the case of simplify_this there is no reason why the function couldn't just be left as a stub that calls simplify with a docstring saying that it's recommended to use simplify instead. Also in the case of is_this_zero there isn't any clear harm in leaving the two argument form there and just advising to use something else in the docs.
There was a problem hiding this comment.
That's fair, although both of these are very similar to real warnings that already exist. There are several of examples of existing deprecations that are just renames for clarity (e.g., tensorhead -> tensor_heads). Maybe we could say these weren't really worth it to begin with, though.
The is_this_zero example is basically an inverted version of a current deprecation of Eq (Eq(x) with a single argument is deprecated). I honestly think this one is fine. It's a good example of something that is both confusing and completely unnecessary IMO.
I can definitely try to come up with better examples (suggestions welcome). Or reuse an existing real deprecation as an example, if it is simple enough (although I rather like how these are completely self-contained).
There was a problem hiding this comment.
I've lost count of the number of times I've copied snippets of code from somewhere and seen the deprecation warning about single argument Eq so it was definitely widely used. Having read the rationale for when to deprecate here (which I agree with) I don't think it was a reasonable thing to deprecate.
There's nothing inherently wrong with Eq(x) as a shorthand for Eq(x, 0) especially since in many contexts where an equation should logically be used sympy allows an expression with the implicit meaning that the expression is equal to zero. In that sense you can view Eq(expr) as simply a conversion between two different ways that sympy represents equations.
More importantly though nothing was gained by deprecating the single argument Eq. It didn't make it possible to make other useful changes or prevent the creation of nonsensical objects or anything else. It was just a deprecation for the sake of it. I agree that it would have been a cleaner API design not to allow single argument Eq in the first place but there was no real benefit from trying to change that retrospectively.
As for a better example I would go with something like integrate(Eq(x, y), t). There's a clear rationale there for why it isn't possible to make that work sensibly. There is clear benefit in showing a message to users explaining why that code is probably incorrect. It isn't possible to just "fix the bug" because that requires introducing an integration constant and anyone currently depending on it won't expect that. If we add the symbol then that's incompatible with the rest of what integrate does and also we have to choose what symbol to use which should really be handled by the user. The code is broken and gives mathematically incorrect results in a way that cannot be fixed in general on SymPy's side without the user updating their code in some way. Ideally this should raise an error but since some code might depend on this we start with a deprecation warning.
There was a problem hiding this comment.
I think the boat's already sailed on single-argument Eq. Personally I think it was fine because Eq(x) is just as likely to be a typo, and explicitly writing Eq(x, 0) isn't hard. It's not unlike how a long time ago we used to make __call__ do substitution but we removed it because people were more likely to mistype multiplication.
I'll think if there are better examples that can be used here. Like I said I do rather like that these examples are small and self-contained, even if they are imaginary (people can look at the real deprecations if they want a more concrete example).
|
I've been through this and left a few comments but broadly I think this is good. Regardless of the comments above I'd say it's fine to merge apart from two points:
I'd rather have a new e.g. |
|
I explained the The stacklevel thing was admittedly a bit of an experiment. The reality is that quite often it's impossible to use a consistent stacklevel, because there's no single call stack to a function. Really the warnings module is very limited in this regard. To be sure, though, it did help me catch several instances where the stacklevel can be set correctly and it just wasn't. However, the other changes to
The warnings tests seem to be working alright for me in pytest, although somehow the XFAIL decorator seems to have broken. I'll look into it. |
|
The pypy issue is because pypy has a different number of stack frames, which is due to the fact that it implements functools.lru_cache in pure Python. This is definitely the sort of annoyance you get with the stacklevel argument. If it becomes even more annoying in the future, we might want to remove it (or alternately, try to come up with some more intelligent logic to automatically set the stacklevel). For now, I'm going to just skip the stacklevel tests for these functions that are cached. The worst that happens when the stacklevel is wrong is you see an unrelated line of code in the warning message, which is also what happens by default when you call |
This fail when the cache is turned off and they also fail in pypy because pypy uses a pure Python implementation of functools.lru_cache. The cache kind of breaks these deprecations anyway (the deprecation can only be triggered exactly once before it is "cached away"). But I'm not really in the mood to try to figure out how to make the cache ignore things that trigger warnings.
Fair enough as long as this has been tested with pytest. I personally only use pytest for running the tests and have dealt with a number of issues in the past that resulted from inconsistencies where someone changed something that didn't show up under the bin/test runner. Fixing lots of warnings was one of them which was why I originally added the I'm not sure why these warnings are not seen under bin/test for example: Lines 7 to 12 in 2c9c12d If you comment that out then you can see lots of warnings under pytest that you don't see under bin/test. Maybe it's because bin/test doesn't activate PendingDeprecationWarning:
$ pytest sympy/physics/quantum/
...
========================================= warnings summary ==========================================
sympy/physics/quantum/tests/test_dagger.py::test_numpy_dagger
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/tests/test_dagger.py:65: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
a = np.matrix([[1.0, 2.0j], [-1.0j, 2.0]])
sympy/physics/quantum/tests/test_density.py: 4 warnings
sympy/physics/quantum/tests/test_matrixutils.py: 1 warning
sympy/physics/quantum/tests/test_represent.py: 32 warnings
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/matrixutils.py:52: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
return np.matrix(m.tolist(), dtype=dtype)
sympy/physics/quantum/tests/test_density.py::test_entropy
sympy/physics/quantum/tests/test_density.py::test_entropy
sympy/physics/quantum/tests/test_represent.py::test_format_numpy
sympy/physics/quantum/tests/test_represent.py::test_format_numpy
sympy/physics/quantum/tests/test_represent.py::test_format_numpy
sympy/physics/quantum/tests/test_represent.py::test_format_numpy
sympy/physics/quantum/tests/test_represent.py::test_format_numpy
sympy/physics/quantum/tests/test_represent.py::test_format_numpy
sympy/physics/quantum/tests/test_represent.py::test_format_numpy
/home/oscar/current/sympy/sympy.git/38venv/lib/python3.8/site-packages/numpy/matrixlib/defmatrix.py:69: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
return matrix(data, dtype=dtype, copy=False)
sympy/physics/quantum/tests/test_density.py: 4 warnings
sympy/physics/quantum/tests/test_matrixutils.py: 1 warning
sympy/physics/quantum/tests/test_represent.py: 32 warnings
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/matrixutils.py:65: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
return sparse.csr_matrix(np.matrix(m.tolist(), dtype=dtype))
sympy/physics/quantum/tests/test_matrixutils.py::test_to_numpy
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/tests/test_matrixutils.py:32: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
result = np.matrix([[1, 2], [3, 4]], dtype='complex')
sympy/physics/quantum/tests/test_matrixutils.py::test_matrix_tensor_product
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/tests/test_matrixutils.py:52: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
numpyl1 = np.matrix(l1.tolist())
sympy/physics/quantum/tests/test_matrixutils.py::test_matrix_tensor_product
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/tests/test_matrixutils.py:53: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
numpyl2 = np.matrix(l2.tolist())
sympy/physics/quantum/tests/test_matrixutils.py::test_matrix_tensor_product
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/tests/test_matrixutils.py:64: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
numpyl2 = np.matrix(l3.tolist())
sympy/physics/quantum/tests/test_matrixutils.py::test_matrix_tensor_product
/home/oscar/current/sympy/sympy.git/sympy/physics/quantum/tests/test_matrixutils.py:75: PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.
numpyl2 = np.matrix(vec.tolist())
-- Docs: https://docs.pytest.org/en/stable/warnings.html
====================== 274 passed, 2 xfailed, 89 warnings in 82.30s (0:01:22) ======================= |
|
Yeah we are only enabling DeprecationWarning in the tests sympy/sympy/testing/runtests.py Line 182 in 2c9c12d Actually pending deprecation warnings are something we didn't really discuss here. They are completely hidden by default (even more hidden than |
They are discussed above. I suggested using them and you said that you didn't think they were useful.
The purpose of PendingDeprecationWarning is to show up in CI for downstream projects. The idea is that downstream developers should see the warnings but not users and developers should fix them so that users don't see them when they become proper warnings or errors later. Since the whole point of PendingDeprecationWarning is to show up in CI pytest makes sure to make them visible. At least |
I'm not sure if that's what I meant. I think they could be useful for instances where we know we want to deprecate something but it's currently not possible for users to replace all instances of the deprecated function. So in that sense, I guess it makes sense that the warning is hidden, since users can't really eliminate the warning by fixing their code. But at the same time, if the warning is hidden, no one will ever see it. It really seems like something that would be better to just be documented than to have a warning. But maybe there are some user workflows where having a warning can be useful, like the pytest workflow you mentioned.
Absolutely. Our tests suite should not allow any warnings, even warnings from other libraries, unless they are explicitly guarded against. This will be challenging, though, as a lot of external libraries tend to emit warnings for all kinds of things. Frankly seeing how annoying they can be gives me pause for using warnings in SymPy (that and the fact that warnings in Python are broken in a dozen different ways). Anything other than a deprecation warning seems like more of an annoyance than anything. It reminds me of the "overfull hbox" warnings that LaTeX spams and almost everyone just ignores. |
|
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[907895ac] [2c9c12df]
- 223±3ms 131±0.5ms 0.59 large_exprs.TimeLargeExpressionOperations.time_subs
- 14.6±0.03ms 9.60±0.01ms 0.66 matrices.TimeMatrixExpression.time_MatAdd_doit
- 223±0.6μs 106±0.1μs 0.47 matrices.TimeMatrixExpression.time_MatMul
- 14.0±0.01ms 7.31±0.02ms 0.52 matrices.TimeMatrixExpression.time_MatMul_doit
- 4.12±0.01s 311±3ms 0.08 polygon.PolygonArbitraryPoint.time_bench01
+ 3.33±0.02ms 5.53±0.03ms 1.66 solve.TimeMatrixOperations.time_det(4, 2)
+ 3.32±0ms 5.52±0.02ms 1.66 solve.TimeMatrixOperations.time_det_bareiss(4, 2)
+ 37.1±0.09ms 65.5±0.4ms 1.77 solve.TimeMatrixSolvePyDySlow.time_linsolve(1)
+ 37.6±0.1ms 65.5±0.6ms 1.74 solve.TimeMatrixSolvePyDySlow.time_solve(1)
Full benchmark results can be found as artifacts in GitHub Actions |
|
Since CI has now passed I'll say that I'm +1 to just merging this and making any further improvements later. I have reviewed the whole diff but I only had minor suggestions (mentioned above). |
It is needed in some places in library code. But if sympy.testing.pytest is imported in library code, it will be imported too early for it to detect that it is running inside of pytest, which breaks the pytest integration. It also is better there anyway since there are legitimate uses of it in library code (to prevent the same or similar warnings from issuing twice due to recursive calls).
|
I pushed a fix for the pytest tests. Apparently you can't import Other than that, I am also OK with merging. If anyone else has any comments on this, please make a new issue or PR. |
|
The pages are live https://docs.sympy.org/dev/guides/contributing/deprecations.html The links in the warnings won't work until they are in a release because they use 'latest' instead of 'dev'. I will go through all the old deprecation removal issues next week and update them. |
asmeurer
added
the
CZI: Documentation
|
@oscarbenjamin if I redo this PR against the 1.10 branch will it be possible to get it in? |
|
Yes |
References to other Issues or PRs
Fixes #20015
Discussed at #22892
Brief description of what is fixed or changed
I have moved the deprecation policy from the wiki into the documentation. I have also made several updates to it. See
deprecations.md.I added some more text going over what should and should not be deprecated. Please read through the document and let me know if you disagree with anything. It's also somewhat lengthy now, so if you have any suggestions on something that should be trimmed or how it should be better organized, let me know.
I did want to include some examples of what does and doesn't constitute public API. I know we don't have a policy on this yet, so I've hopefully made it clear that this is not strictly codified (but I don't want that to prevent us from saying anything about it).
I opened Clarify what is public and what is private #23037 to discuss this.
I have made the deprecation period officially at least 1 year. Hopefully I have made it clear that this is just a minimum period. Deprecations can last longer than this. The deprecation policy is time-based and not version-based. I have added some justifications for this. Please let me know if you agree with this policy.
I have completely nuked the "deprecation removal issue" stuff. See SymPy Deprecation backlog #22892. Basically no one really understood this, and it was a huge mess.
Instead, we are going to put all information about deprecations in the documentation itself. This will be done in 3 places:
.. deprecated::directive in each docstring.See the document for a description of what should go in each.
Other comments
Additionally, I have done the following.
I've updated the constructor for creating deprecation warnings to be much simpler. It used to be
now it is
The
messageshould contain a description of the deprecated functionality and what it should be replaced with. Multiline strings with code blocks are be encouraged. Theactive_deprecations_targetshould be a cross-reference target in the "active deprecations" page which will be used to automatically generate a link to the document at the end of the warning.deprecated_since_versionis the same as before. It should be the version of the next release (the version in release.py minus.dev). Library code should use thesympy_deprecation_warningfunction to issue a warning rather than theSymPyDeprecationWarningconstructor directly (previously you had to callSymPyDeprecationWarning(...).warn(), but it was easy to forget thewarn()part).(yes this is itself a backwards incompatible change. But
SymPyDeprecationWarningshould not be used by non-library code).You can also use
@deprecatedbut it now has the exact same signature assympy_deprecation_warning, and there is no effective difference between using it and just callingsympy_deprecation_warning()at the top of the function.See the diff for tons of examples of this.
Update all existing deprecations to use the above format. That means replacing the existing "deprecation removal" issues with text in the active deprecations document, rewriting all the warning text to be more descriptive and to use the new
SymPyDeprecationWarningsignature, and adding.. deprecation:: <version>directives to all relevant docstrings.I have not removed any currently active deprecations (except for one minor thing that was missed in Deprecation removals #22952). Some deprecations here are quite old, but also a lot of them were documented extremely poorly, to the point that I don't know how anyone could ever tell what they were actually supposed to do to fix their code. So I would suggest keeping the existing deprecations for now, especially if we can get this into the next release.
I have updated the
warnsandwarns_deprecated_sympytest context managers:stacklevelargument is set correctly.stacklevelshould be set so that the lien of code for a warning is the user line of code. The default value forsympy_deprecation_warningis set so that you usually don't have to worry about this, but you will need to increase it if you issue the warning from some helper function. If there's no way do this consistently, you can disable it withwarns(test_stacklevel=False).warns()no longer ignores all other warnings. This was hiding a use of deprecated code in the tests (plot_implicit() relies on the string fallback insympify(). I have left this as XFAIL because I don't know how to fix it. It seems to depend heavily on the poor design ofexerimental_lambdifyand the interval math).Once this is merged, I will delete the old policy from the wiki and update any references to it, and close all "deprecation removal" issues, replacing them with links to the new document.
Release Notes