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

Remove chemistry dep warning #4466

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Conversation

kratman
Copy link
Contributor

@kratman kratman commented Sep 25, 2024

Description

Remove the old deprecation warning for the chem argument. It was from March 2023, so over a year old

Key checklist:

  • No style issues: $ 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)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ 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:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

I know that this is quite old and probably isn't in use anywhere either, but could you please add a CHANGELOG entry, too?

@kratman
Copy link
Contributor Author

kratman commented Sep 25, 2024

I know that this is quite old and probably isn't in use anywhere either, but could you please add a CHANGELOG entry, too?

Yeah I was planning on it, I just needed to create the PR so I had the correct number

@agriyakhetarpal
Copy link
Member

I know that this is quite old and probably isn't in use anywhere either, but could you please add a CHANGELOG entry, too?

Yeah I was planning on it, I just needed to create the PR so I had the correct number

You might like https://ichard26.github.io/next-pr-number/, I use it quite frequently for this purpose

@kratman kratman enabled auto-merge (squash) September 25, 2024 16:27
Comment on lines +7 to +8
- Added phase-dependent particle options to LAM
([#4369](https://github.com/pybamm-team/PyBaMM/pull/4369))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Added phase-dependent particle options to LAM
([#4369](https://github.com/pybamm-team/PyBaMM/pull/4369))
- Added phase-dependent particle options to LAM ([#4369](https://github.com/pybamm-team/PyBaMM/pull/4369))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of those render why does this matter?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would split up into a new line on https://pybamm.org/changelog/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already a bulleted list in that page, if that does not work then there is something wrong with the library used for it

Copy link
Member

Choose a reason for hiding this comment

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

Now that I notice, that page is outdated and does not show any results since 24.1 was last released

Copy link
Member

Choose a reason for hiding this comment

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

In other words, the Goldmark renderer for Hugo has an option to wrap text, will check if it's enabled

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Thanks @kratman

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.46%. Comparing base (1b6ef03) to head (a979e62).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4466   +/-   ##
========================================
  Coverage    99.46%   99.46%           
========================================
  Files          293      293           
  Lines        22384    22386    +2     
========================================
+ Hits         22264    22266    +2     
  Misses         120      120           

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

@kratman kratman merged commit 1390ea3 into pybamm-team:develop Sep 25, 2024
26 checks passed
@agriyakhetarpal agriyakhetarpal deleted the feat/removeChem branch September 25, 2024 18:36
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.

3 participants