-
Notifications
You must be signed in to change notification settings - Fork 344
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
Pickleable particles #1122
Pickleable particles #1122
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1122 +/- ##
==========================================
- Coverage 96.91% 96.90% -0.02%
==========================================
Files 70 70
Lines 6930 6936 +6
==========================================
+ Hits 6716 6721 +5
- Misses 214 215 +1
Continue to review full report at Codecov.
|
Having had time to think this through on the evening doggo walk, Nope, I got nothing. |
I can write a somewhat contrived test using scipy.optimize (I'd have to make up a fitting function that uses a particle instance), but I'm pretty sure that if your new version is pickleable (which it seems like it is, if @namurphy 's test is passing now) then I think it should work with scipy too? |
It should! If it's more complex than necessary, don't bother :) |
So, my attempts at writing a test function that would demonstrate the issue with
But resulted in this new error
In summary, I think this fixes the pickling issue, although I'm not sure if the new error is related to your fix or was just always waiting after the first error. |
I'd still appreciate the code for that! If it's doing complex setup beforehand, that's fine; so long as it reproduces. Maybe I can pinpoint the reason why it's crashing. |
To test in-vivo in the thomson fitting, just set But, I think I got a test working! The code below fits two models: one with a Particle in it and one without. On the main branch version, only the first fit executes, then the same "Not pickleable" error is raised during the second one. But, on your branch, both execute! This could be rearranged into a test, but it's computationally intensive and I think redundant with the pickling tests that already exist so I'd be inclined to leave it out of the official tests? Don't worry about that second error above: that was my fault (not a problem with the code). from lmfit import Parameters, Model
def _mock_model(x, settings=None, **params):
particle = settings['particle']
a = params['a']
b = params['b']
return particle.integer_charge*np.exp(-(x-a)**2/b**2)
def _mock_model2(x, settings=None, **params):
a = params['a']
b = params['b']
return np.exp(-(x-a)**2/b**2)
def test_differential_evolution_oarallelization_with_particles():
x = np.linspace(1, 5, num=1000)
particle = Particle('H+')
a = 3
b = 1
ydata = particle.integer_charge*np.exp(-(x-a)**2/b**2)*np.random.normal(loc=1.0, scale=0.01, size=x.size)
params = Parameters()
params.add('a', 5, min=0.1, max=10, vary=True)
params.add('b', 5, min=0.1, max=10, vary=True)
fit_kws = {'workers':-1, 'updating':'deferred'}
# TEST 1: Create model without particle. This should work in parallel
model = Model(_mock_model2, intependent_vars=["x"],
settings=None)
res = model.fit(ydata, params, x=x, fit_kws=fit_kws, method='differential_evolution')
print(res.best_values)
# TEST 2: Create model with the particle. Should fail if particles cannot
# be pickled
model = Model(_mock_model, intependent_vars=["x"],
settings={'particle':particle})
res = model.fit(ydata, params, x=x, fit_kws=fit_kws, method='differential_evolution')
print(res.best_values) I was inserting this test at the bottom of |
Yeah, I suspect it's fine to leave it out. I can confirm that both pass on my branch. Shall we merge it, then? :) |
Sounds good to me! What does @namurphy think? |
Closes #1011!
I do not fully understand why this works. But it seems that it does.
I was tinkering with slicing of IonizationState as part of #1059 (@namurphy I have actual arguments for that, and it's just the first of many travesties I want to do to IonizationState). I then somehow stumbled upon this, that got me thinking, I started looking at
Particle.__dict__
, noticed thatjson.dumps(particle)
complains about_PeriodicTable
contained within that__dict__
, realized that's a namedtuple, thought hey, if namedtuples have trouble pickling (I don't understand why they would, that's my whole problem with this solutin!), maybe dataclasses wouldn't. And guess what, they don't. And the xfailing tests started passing.@pheuer could you look at that issue you had previously with scipy.optimize, maybe even add a smaller test case of that somewhere as a failing (
pytest.mark.xfail
)? I'd like to check again. Maybe this got fixed.