PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

[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`

Open Akhil-Sharma30 opened this issue 1 year ago • 9 comments

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)
  • [x] 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

Akhil-Sharma30 avatar Jan 21 '24 11:01 Akhil-Sharma30

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

Done.

Akhil-Sharma30 avatar Jan 21 '24 12:01 Akhil-Sharma30

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

agriyakhetarpal avatar Jan 21 '24 12:01 agriyakhetarpal

@agriyakhetarpal

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

allcontributors[bot] avatar Jan 21 '24 12:01 allcontributors[bot]

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

Akhil-Sharma30 avatar Jan 22 '24 09:01 Akhil-Sharma30

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?

Akhil-Sharma30 avatar Mar 10 '24 13:03 Akhil-Sharma30

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.

arjxn-py avatar Mar 11 '24 07:03 arjxn-py

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.

Akhil-Sharma30 avatar Mar 12 '24 07:03 Akhil-Sharma30

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>

Saransh-cpp avatar Mar 12 '24 10:03 Saransh-cpp

Hi @Akhil-Sharma30, any updates on this?

brosaplanella avatar May 13 '24 12:05 brosaplanella