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

Deprecation decorators for functions #4757

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

RohitP2005
Copy link
Contributor

Description

This pull request has implemented deprecation decorators as wrapper for a function's arguement and the fucntion itself and has added -W flag to ensure the deprecation warnings are treated as error

Fixes #2028

Type of change

  • New feature (non-breaking change that adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Deprecation (non-breaking change that deprecates functionality with warnings)

Key checklist:

  • No style issues: $ pre-commit run --all-files (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally)
  • All tests pass: $ python -m pytest -W error (or $ nox -s tests) (ensure warnings are treated as errors to catch deprecation notices)
  • The documentation builds: $ python -m pytest --doctest-plus src (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once using $ nox -s quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas, including deprecation warnings and helper functions.
  • Tests added for any new functionality or to prove that the deprecation warnings are effective.
  • Confirm that any borrowed code from Astropy is appropriately attributed according to its license.

@RohitP2005
Copy link
Contributor Author

Previous Discussions : #4745 (comment)

utils/exceptions.py Outdated Show resolved Hide resolved
src/pybamm/simulation.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

I think one of the main reasons for doing this is to track the date of the deprecation warnings. This does not seem to be in these changes.

Realistically the warnings work as-is, but we want to automate the removal of them. Right now I use git-blame to remove old warnings every 6 months or so. The extra decorators and such are just additional overhead unless we use it for something better than the current warnings

@kratman
Copy link
Contributor

kratman commented Jan 14, 2025

Original issue request: "Replace existing deprecation warnings/errors with deprecation package so tests fail after a few releases."

@RohitP2005
Copy link
Contributor Author

RohitP2005 commented Jan 14, 2025

Okay , I misunderstood that the deprecation package was not required and we needed a pythonic way.
Now i understand @kratman , Thankyou for the clarification .

I will just go ahead with the deprecation package with my next commit

Apologies for the confusion

@RohitP2005
Copy link
Contributor Author

@kratman, I've implemented the deprecation package for four of the existing deprecation warnings. Could you please guide me on any potential improvements?

Regarding renamed and deprecated arguments, it seems the deprecation package doesn't natively handle these cases. Do you have any suggestions on how to approach this? I considered writing custom decorators to manage them. Does that sound like a good plan?

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.66%. Comparing base (8b33589) to head (7f34a0b).
Report is 6 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4757   +/-   ##
========================================
  Coverage    98.66%   98.66%           
========================================
  Files          303      303           
  Lines        23227    23233    +6     
========================================
+ Hits         22916    22922    +6     
  Misses         311      311           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kratman
Copy link
Contributor

kratman commented Jan 17, 2025

Does the package you use allow for tests to fail when the deprecation is old?

Comment on lines 181 to 182
msg = "pybamm.simulation.set_up_and_parameterise_experiment is deprecated and not meant to be accessed by users."
warnings.warn(msg, DeprecationWarning, stacklevel=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use warnings.warn here since we have already used the deprecated decorator above? The msg is repetitive, as already defined above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I planned on removing them on the next commit . once my Eric reviewed it and was ok with the implementation.

Comment on lines 270 to 271
msg = (
"pybamm.set_parameters is deprecated and not meant to be accessed by users."
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@RohitP2005
Copy link
Contributor Author

Does the package you use allow for tests to fail when the deprecation is old?

No, the deprecation package does not natively support automatically failing tests when a deprecation is old

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.

Use deprecation package for deprecation warnings
3 participants