[Bug]: `pybamm.Simulation.set_parameters`, `pybamm.Simulation.set_up_and_parameterise_experiment` and `pybamm.Simulation.set_up_and_parameterise_model_for_experiment` made private in `simulation.py` by Akhil-Sharma30 · Pull Request #3752 · pybamm-team/PyBaMM

Akhil-Sharma30 · 2024-01-21T11:53:31Z

Description

pybamm.Simulation.set_parameters, pybamm.Simulation.set_up_and_parameterise_experiment and pybamm.Simulation.set_up_and_parameterise_model_for_experiment made private in pybamm/simulation.py

Fixes #3751

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

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

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

valentinsulzer

Thanks @Akhil-Sharma30

valentinsulzer

You will need to also update the method names wherever they are called (which should only be inside the Simulation class)

agriyakhetarpal

Thanks @Akhil-Sharma30 for the fix!

Edit: yes, I missed Valentin's note. Please update the methods too

This pull request needs extra changes, which I overlooked

Akhil-Sharma30 · 2024-01-21T12:26:08Z

You will need to also update the method names wherever they are called (which should only be inside the Simulation class)

Done.

agriyakhetarpal · 2024-01-21T12:36:58Z

@all-contributors please add @Akhil-Sharma30 for docs

allcontributors · 2024-01-21T12:37:08Z

@agriyakhetarpal

I've put up a pull request to add @Akhil-Sharma30! 🎉

arjxn-py

Thanks @Akhil-Sharma30, should be good to merge after Valentin's approval.

Saransh-cpp

Ideally, we should deprecate these functions before making them private, given that they are visible in the API docs, but this might be fine if not a lot of users are using them. However, this should be documented in the CHANGELOG (as a breaking change) and the tests should be fixed (you can look at the logs in GH Actions to see where these functions are being used).

Akhil-Sharma30 · 2024-01-22T09:45:52Z

Ideally, we should deprecate these functions before making them private, given that they are visible in the API docs, but this might be fine if not a lot of users are using them. However, this should be documented in the CHANGELOG (as a breaking change) and the tests should be fixed (you can look at the logs in GH Actions to see where these functions are being used).

Added a deprecated warning inside the function and made them public and also mentioned this in the CHANGELOG.md

…ent` in simulation

…ulation

Saransh-cpp · 2024-01-23T10:36:49Z

pybamm/simulation.py

+        msg = "pybamm.simulation.set_up_and_parameterise_experiment is not meant to be accessed directly."
+        warn(msg, DeprecationWarning)


Given that not a lot of users are using these methods and we want to make them private in the next release - we could create dummy public methods that throw NameError on being invoked -

def set_up_and_parameterise_experiment(self): """ A note here for users that this should not be used and is not private with the name _set_up_and_parameterise_experiment """ raise NameError(...)

Just suggestions. We should go ahead with the strategy that most maintainers agree with.

happy to take suggestions and do what is decided upon

I don't think it's a good idea to make them private right in the next release, since it doesn't conform to our policy of three releases – even if the popularity of this API not being relatively high is the question. Keeping them public plus raising a DeprecationWarning should be enough, a NameError sounds a bit odd IMO (I don't think we have used this before).

Saransh-cpp

Sorry for taking too long to review, but the deprecation path sounds good to me!

Saransh-cpp · 2024-02-29T10:28:05Z

pybamm/simulation.py


    def set_up_and_parameterise_experiment(self):
        """
+        This is a helper function.


Suggested change

This is a helper function.

Saransh-cpp · 2024-02-29T10:28:53Z

CHANGELOG.md


 ## Breaking changes

+- Added a deprecated warning for `pybamm.Simulation.set_parameters`, `pybamm.Simulation.set_up_and_parameterise_experiment` and `pybamm.Simulation.set_up_and_parameterise_model_for_experiment` functions in `pybamm.simulation.py`. ([#3752](https://github.com/pybamm-team/PyBaMM/pull/3752))


You'll also need to shift it to the unreleased section

Suggested change

- Added a deprecated warning for `pybamm.Simulation.set_parameters`, `pybamm.Simulation.set_up_and_parameterise_experiment` and `pybamm.Simulation.set_up_and_parameterise_model_for_experiment` functions in `pybamm.simulation.py`. ([#3752](https://github.com/pybamm-team/PyBaMM/pull/3752))

- Deprecated `pybamm.Simulation.set_parameters`, `pybamm.Simulation.set_up_and_parameterise_experiment` and `pybamm.Simulation.set_up_and_parameterise_model_for_experiment` functions in `pybamm.simulation.py`. ([#3752](https://github.com/pybamm-team/PyBaMM/pull/3752))

Saransh-cpp · 2024-02-29T10:30:22Z

pybamm/simulation.py

 from datetime import timedelta
 from pybamm.util import have_optional_dependency
 from typing import Optional
+from warnings import warn


Nitpicking here, but we should keep this uniform throughout the codebase -

Suggested change

from warnings import warn

import warnings

and then warnings.warn everywhere

Saransh-cpp · 2024-02-29T13:49:09Z

pybamm/simulation.py

        This needs to be done here and not in the Experiment class because the nominal
        cell capacity (from the parameters) is used to convert C-rate to current.
        """
+        msg = "pybamm.simulation.set_up_and_parameterise_experiment is not meant to be accessed directly."


Suggested change

msg = "pybamm.simulation.set_up_and_parameterise_experiment is not meant to be accessed directly."

msg = "pybamm.simulation.set_up_and_parameterise_experiment is deprecated and not meant to be accessed by users"

Saransh-cpp · 2024-02-29T13:49:34Z

pybamm/simulation.py


    def set_up_and_parameterise_model_for_experiment(self):
        """
+        This is a helper function.


Suggested change

This is a helper function.

Saransh-cpp · 2024-02-29T13:49:51Z

pybamm/simulation.py

        This increases set-up time since several models to be processed, but
        reduces simulation time since the model formulation is efficient.
        """
+        msg = "pybamm.simulation.set_up_and_parameterise_model_for_experiment is not meant to be accessed directly."


Suggested change

msg = "pybamm.simulation.set_up_and_parameterise_model_for_experiment is not meant to be accessed directly."

msg = "pybamm.simulation.set_up_and_parameterise_model_for_experiment is deprecated not meant to be accessed by users"

Saransh-cpp · 2024-02-29T13:50:06Z

pybamm/simulation.py

        A method to set the parameters in the model and the associated geometry.
        """
-
+        msg = "pybamm.set_paramters is meant to be accessed directly."


Suggested change

msg = "pybamm.set_paramters is meant to be accessed directly."

msg = "pybamm.set_paramters is deprecated and not meant to be accessed by users"

Akhil-Sharma30 · 2024-03-10T13:12:50Z

I am not able to reset my branch I made a mess of it. Could someone provide me instruction on how to fix this issue?

arjxn-py · 2024-03-11T07:09:29Z

I am not able to reset my branch I made a mess of it. Could someone provide me instruction on how to fix this issue?

Can you provide some logs while you're trying to reset your branch?

Although assuming that you might be getting merge conflicts while trying to reset to your branch i.e. :

I have resolved a merge conflict in simulation.py and while doing that i have also removed import warn by mistake. Sorry, I'd now recommend you to make sure that your local working tree is clean before resetting your branch and post logs in case you're still not able to. Plus also add import warn again to fix the failing CI.

Akhil-Sharma30 · 2024-03-12T07:00:24Z

I am not able to reset my branch I made a mess of it. Could someone provide me instruction on how to fix this issue?

Can you provide some logs while you're trying to reset your branch?

Although assuming that you might be getting merge conflicts while trying to reset to your branch i.e. :

I have resolved a merge conflict in simulation.py and while doing that i have also removed import warn by mistake. Sorry, I'd now recommend you to make sure that your local working tree is clean before resetting your branch and post logs in case you're still not able to. Plus also add import warn again to fix the failing CI.

Thanks for the help. I by mistake deleted the branch but when I was trying to bring the branch locally it was causing issue so I tried the gh pr checkout 3752 it showed me this error GraphQL: Could not resolve to a PullRequest with the number of 3752. (repository.pullRequest)

If possible could you suggest me any method to bring that branch locally again so that I can do the changes.

Saransh-cpp · 2024-03-12T10:38:30Z

I am not very familiar with the GitHub CLI, but with git you should be able to do something like this -

git fetch --all   # fetch everything locally (there must be a way to just fetch 1 branch)
git checkout <your_branch>

brosaplanella · 2024-05-13T12:57:18Z

Hi @Akhil-Sharma30, any updates on this?

kratman · 2024-10-01T17:26:41Z

@Akhil-Sharma30 This has been open for quite a while are you working on this still? If not we should close it because there have been a lot of changes to PyBaMM since this was opened

Akhil-Sharma30 · 2024-10-02T17:14:34Z

@Akhil-Sharma30 This has been open for quite a while are you working on this still? If not we should close it because there have been a lot of changes to PyBaMM since this was opened

Apologize for the delay, I was busy with some work I will fix the changes in 1-2 days maximum.

agriyakhetarpal · 2024-10-03T12:54:57Z

Could you also resolve the merge conflicts, @Akhil-Sharma30?

Akhil-Sharma30 · 2024-10-03T13:16:42Z

Could you also resolve the merge conflicts, @Akhil-Sharma30?

If possible could you guide me from where can I solve these conflict?

kratman · 2024-10-03T13:41:42Z

Could you also resolve the merge conflicts, @Akhil-Sharma30?

If possible could you guide me from where can I solve these conflict?

@Akhil-Sharma30 Honestly, your change is ~10 lines. It is probably easier to just redo this in a fresh PR. Just check out develop and redo the small changes

Akhil-Sharma30 · 2024-10-04T06:35:58Z

Could you also resolve the merge conflicts, @Akhil-Sharma30?

If possible could you guide me from where can I solve these conflict?

@Akhil-Sharma30 Honestly, your change is ~10 lines. It is probably easier to just redo this in a fresh PR. Just check out develop and redo the small changes

Okay I have created a fresh PR for this. Just wanted to know if this pybamm.Simulation.set_up_and_parameterise_model_for_experiment func is removed from the simulation.py before a push the new PR?

Saransh-cpp · 2024-10-04T09:25:10Z

Hi, it would be easier to open a new PR and request a review / ask your queries there. We'll be able to help you better in a fresh space. This PR is a bit too tangled now.

Akhil-Sharma30 · 2024-10-04T10:12:29Z

I have created a new PR for this fix #4489

Akhil-Sharma30 requested a review from a team as a code owner January 21, 2024 11:53

valentinsulzer approved these changes Jan 21, 2024

View reviewed changes

valentinsulzer requested changes Jan 21, 2024

View reviewed changes

agriyakhetarpal previously approved these changes Jan 21, 2024

View reviewed changes

allcontributors bot mentioned this pull request Jan 21, 2024

docs: add Akhil-Sharma30 as a contributor for doc #3753

Merged

arjxn-py previously approved these changes Jan 21, 2024

View reviewed changes

arjxn-py requested a review from valentinsulzer January 21, 2024 12:38

Saransh-cpp requested changes Jan 21, 2024

View reviewed changes

Akhil-Sharma30 force-pushed the simulation_func_private branch 2 times, most recently from a2baa6b to 4672625 Compare January 22, 2024 10:30

Akhil-Sharma30 added 4 commits January 22, 2024 16:02

Add deprecated warning for `set_up_and_parameterise_model_for_experim…

5917f2a

…ent` in simulation

Add deprecated warning for set_up_and_parameterise_experimentin sim…

f4188c1

…ulation

Add deprecated warning for set_parameters in simulation

4aa147c

Add simulation func info in breaking changes inside CHANGELOG.md

ad30142

Akhil-Sharma30 force-pushed the simulation_func_private branch from 63f499a to ad30142 Compare January 22, 2024 10:33

style: pre-commit fixes

a6848ee

Saransh-cpp reviewed Jan 23, 2024

View reviewed changes

arjxn-py self-requested a review February 12, 2024 08:47

Saransh-cpp requested changes Feb 29, 2024

View reviewed changes

Saransh-cpp mentioned this pull request Feb 29, 2024

Make get_spatial_var private #3755

Merged

8 tasks

Merge branch 'develop' into simulation_func_private

40d152d

Akhil-Sharma30 dismissed stale reviews from arjxn-py and agriyakhetarpal via 40d152d September 18, 2024 21:47

kratman added the needs-reply Needs further information from the author and may be closed if no response is received label Oct 1, 2024

github-actions bot removed the needs-reply Needs further information from the author and may be closed if no response is received label Oct 2, 2024

Akhil-Sharma30 added 2 commits October 2, 2024 23:00

Fix: change the deprecation message in simulation.py

6ae475d

Fix: Move the deprecated warning to unreleased section

86a7056

Akhil-Sharma30 mentioned this pull request Oct 4, 2024

[Bug]: pybamm.Simulation.set_parameters and pybamm.Simulation.set_up_and_parameterise_experiment made private in simulation.py #4489

Merged

8 tasks

Saransh-cpp closed this Oct 4, 2024

		msg = "pybamm.simulation.set_up_and_parameterise_experiment is not meant to be accessed directly."
		warn(msg, DeprecationWarning)


		## Breaking changes

		- Added a deprecated warning for `pybamm.Simulation.set_parameters`, `pybamm.Simulation.set_up_and_parameterise_experiment` and `pybamm.Simulation.set_up_and_parameterise_model_for_experiment` functions in `pybamm.simulation.py`. ([#3752](https://github.com/pybamm-team/PyBaMM/pull/3752))

	- Added a deprecated warning for `pybamm.Simulation.set_parameters`, `pybamm.Simulation.set_up_and_parameterise_experiment` and `pybamm.Simulation.set_up_and_parameterise_model_for_experiment` functions in `pybamm.simulation.py`. ([#3752](https://github.com/pybamm-team/PyBaMM/pull/3752))
	- Deprecated `pybamm.Simulation.set_parameters`, `pybamm.Simulation.set_up_and_parameterise_experiment` and `pybamm.Simulation.set_up_and_parameterise_model_for_experiment` functions in `pybamm.simulation.py`. ([#3752](https://github.com/pybamm-team/PyBaMM/pull/3752))

	msg = "pybamm.set_paramters is meant to be accessed directly."
	msg = "pybamm.set_paramters is deprecated and not meant to be accessed by users"

Uh oh!

Conversation

Akhil-Sharma30 commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist:

Further checks:

Uh oh!

valentinsulzer left a comment

Choose a reason for hiding this comment

Uh oh!

valentinsulzer left a comment

Choose a reason for hiding this comment

Uh oh!

agriyakhetarpal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Akhil-Sharma30 commented Jan 21, 2024

Uh oh!

agriyakhetarpal commented Jan 21, 2024

Uh oh!

allcontributors bot commented Jan 21, 2024

Uh oh!

arjxn-py left a comment

Choose a reason for hiding this comment

Uh oh!

Saransh-cpp left a comment

Choose a reason for hiding this comment

Uh oh!

Akhil-Sharma30 commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Akhil-Sharma30 Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saransh-cpp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Akhil-Sharma30 commented Mar 10, 2024

Uh oh!

arjxn-py commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akhil-Sharma30 commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saransh-cpp commented Mar 12, 2024

Uh oh!

brosaplanella commented May 13, 2024

Uh oh!

kratman commented Oct 1, 2024

Uh oh!

Akhil-Sharma30 commented Oct 2, 2024

Uh oh!

agriyakhetarpal commented Oct 3, 2024

Uh oh!

Akhil-Sharma30 commented Oct 3, 2024

Uh oh!

kratman commented Oct 3, 2024

Akhil-Sharma30 commented Jan 21, 2024 •

edited

Loading

agriyakhetarpal left a comment •

edited

Loading

Akhil-Sharma30 commented Jan 22, 2024 •

edited

Loading

Akhil-Sharma30 Jan 25, 2024 •

edited

Loading

arjxn-py commented Mar 11, 2024 •

edited

Loading

Akhil-Sharma30 commented Mar 12, 2024 •

edited

Loading

Akhil-Sharma30 commented Oct 4, 2024 •

edited

Loading