Removing old arguments #3682
Removing old arguments #3682
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3682 +/- ##
========================================
Coverage 99.59% 99.59%
========================================
Files 258 257 -1
Lines 20827 20796 -31
========================================
- Hits 20742 20712 -30
+ Misses 85 84 -1 ☔ View full report in Codecov by Sentry. |
|
Deprecated things are kept in the codebase for at least a year (at least three releases), are we in a position to remove these? I cannot exactly ascertain the duration from the git blame |
Is this documented somewhere? I don't think I have seen that rule and I could not find it with a text search in our guidelines.
The tests I removed were put in May 2023, so sometime around then. |
It's mentioned in the Versioning section of the README. I agree it's a bit difficult to find though and should be communicated more transparently in the Contributing Guide for developers and future contributors. Please feel free to add this note more explicitly at some place where you think it would be apt.
Ah I see, I don't have an opinion on this so I will leave it up to the others to see if removing it now should be fine, or we can come back to merging this PR later at the time of 24.5rc0 |
|
happy to merge this now and have it go in 24.5 |
I will add the test and I also want to do a search for other outdated deprecation warnings. I will update soon |
|
Other deprecations that have to be removed from the codebase in v24.5 are all the entry points from |
kratman
requested review from
brosaplanella,
DrSOKane,
valentinsulzer,
TomTranter and
martinjrobins
as code owners
@agriyakhetarpal Since you are more familiar with the entry points than I am, feel free to push that change to this branch. |
| @staticmethod | ||
| def process_steps(unprocessed_steps, period, temp): | ||
| processed_steps = {} | ||
| for step in unprocessed_steps: | ||
| if repr(step) in processed_steps: | ||
| continue | ||
| elif isinstance(step, str): | ||
| processed_step = pybamm.step.string(step) | ||
| elif isinstance(step, pybamm.step._Step): | ||
| processed_step = step | ||
| else: | ||
| raise TypeError("Operating conditions must be a Step object or string.") | ||
|
|
||
| if processed_step.period is None: | ||
| processed_step.period = period | ||
| if processed_step.temperature is None: | ||
| processed_step.temperature = temp | ||
|
|
||
| processed_steps[repr(step)] = processed_step | ||
| return processed_steps |
There was a problem hiding this comment.
@rtimms I extracted this method so it was easily testable. I added a test for it.
kratman
requested review from
Saransh-cpp,
agriyakhetarpal,
arjxn-py and
a team
as code owners
|
Actually, |
* Removing old arguments * A couple extra fixes * Removing more old warnings, tests failing * Remove old tests * Extract method and add a test * Remove parameters CLI and associated files --------- Co-authored-by: Agriya Khetarpal <[email protected]>
Description
These arguments have been deprecated for a while. I figured it would be better to remove them to reduce code and error handling.
Type of change
Minor cleanup.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: