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

MBCn: base_kws_vars copied to avoid global changes #1999

Merged
merged 3 commits into from
Nov 19, 2024
Merged

Conversation

coxipi
Copy link
Contributor

@coxipi coxipi commented Nov 19, 2024

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGELOG.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • Copy base_kws_vars inside the adjust function so that it doesn't change the dict globally.

Does this PR introduce a breaking change?

No

Other information:

@github-actions github-actions bot added the sdba Issues concerning the sdba submodule. label Nov 19, 2024
@coxipi
Copy link
Contributor Author

coxipi commented Nov 19, 2024

Reassigning a dict in a function works btw:

import copy 
dd = {"a":1, "b":2}
def ff(dd): 
    dd = copy.deepcopy(dd)
    dd["a"] = 2
ff(dd)
dd
>>> {"a":1, "b":2}

@coxipi coxipi requested a review from Zeitsperre November 19, 2024 16:02
Copy link
Collaborator

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Approved for additional tests label Nov 19, 2024
@coxipi
Copy link
Contributor Author

coxipi commented Nov 19, 2024

@tlogan2000 I will be merging this once the tests are passed to address the bug you have found, thanks.

@coveralls
Copy link

Coverage Status

coverage: 89.399% (+0.001%) from 89.398%
when pulling d31f51e on mbcn_params_dict_local
into 5c4cf5f on main.

@coxipi coxipi merged commit 6b5904e into main Nov 19, 2024
30 checks passed
@coxipi coxipi deleted the mbcn_params_dict_local branch November 19, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests sdba Issues concerning the sdba submodule.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants