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

Fix deepcopy of user data #837

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Conversation

mgunyho
Copy link
Contributor

@mgunyho mgunyho commented Feb 4, 2023

Description

I was making use of the user_data attribute of a Parameter, and noticed that it is not deep-copied when using the copy() method of Parameters. Here is a simple example:

import lmfit

def func(x, a):
    return a * x

model = lmfit.Model(func)

params = model.make_params()
params.get("a").user_data = {"foo": 123 }

params2 = params.copy()
params2.get("a").user_data["foo"] = 456

print(params.get("a").user_data) # prints {'foo': 456}

This PR fixes this behavior.

Type of Changes
  • Bug fix
  • New feature
  • Refactoring / maintenance
  • Documentation / examples
Tested on

Python: 3.10.9 (main, Dec 08 2022, 14:49:06) [GCC]

lmfit: 1.1.0.post1+g1a3b1009, scipy: 1.10.0, numpy: 1.24.1, asteval: 0.9.28, uncertainties: 3.1.7

Verification

Have you

  • included docstrings that follow PEP 257? - N/A
  • referenced existing Issue and/or provided relevant link to mailing list? - N/A
  • verified that existing tests pass locally?
  • verified that the documentation builds locally? - N/A, no changes to docs
  • squashed/minimized your commits and written descriptive commit messages? - let me know if you want me to squash them into one commit
  • added or updated existing tests to cover the changes?
  • updated the documentation and/or added an entry to the release notes (doc/whatsnew.rst)?
  • added an example? - N/A

exp_attr_values_A = ('a', 10.0, True, -100.0, 100.0, None, 5.0, 1)
exp_attr_values_B = ('b', 20.0, False, -250.0, 250.0, "2.0*a", 25.0, 2.5)
exp_attr_values_B = ('b', 20.0, False, -250.0, 250.0, "2.0*a", 25.0, {'test': 123})
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the purpose of this change - why would one want to make things fail when user_data is a dictionary? Or am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, the test covers a case that was not covered before, where the user data is a mutable object such as a dict or list. See the example code in the PR: modifying the user data of the copied params2 modifies the user data of the original params.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean. In the current test "user_data" was an int/float value in both cases, replacing one with a more complicated type makes sense!

@@ -109,7 +109,7 @@ def __deepcopy__(self, memo):
param.correl = par.correl
param.init_value = par.init_value
param.expr = par.expr
param.user_data = par.user_data
param.user_data = deepcopy(par.user_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a real issue with doing this, but why is it explicitly needed here and not for any of the other attributes - that doesn't make complete sense to me, but admittedly I haven't looked carefully yet...

Copy link
Contributor Author

@mgunyho mgunyho Feb 6, 2023

Choose a reason for hiding this comment

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

I am quite new to lmfit, so I'm not sure which of the other attributes are mutable types, maybe correl is for example. I agree that it looks a bit asymmetric, but this at least fixes this bug.

@newville
Copy link
Member

newville commented Feb 7, 2023

@mgunyho @reneeotten Thanks - I do agree that user_data is the most likely to be mutable. I think we should do a deepcopy on user_data and correl and probably everything in the user_defined_symbols, as with

unique_symbols = {key: deepcopy(self._asteval.symtable[key]) for key in self._asteval.user_defined_symbols()}

@mgunyho
Copy link
Contributor Author

mgunyho commented Feb 7, 2023

That seems reasonable. I am not familiar with the internals of Parameter, and I'm not sure if/how the tests should be updated to check that the behavior wrt _asteval or the other attributes is correct. You should be able to add commits to this PR, so feel free to do so if you don't want to make a separate one.

@newville
Copy link
Member

@mgunyho @reneeotten will merge and update #844

@newville newville merged commit d2ba8e9 into lmfit:master Feb 24, 2023
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