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

Remove lambda functions #1013

Merged
merged 8 commits into from
Feb 16, 2021
Merged

Conversation

namurphy
Copy link
Member

This pull request removes some lambda expressions from plasmapy.particles and plasmapy.utils because lambda expressions cannot be pickled. This partially address #1011. However, more would need to be done in order to pickle Particle instances. I left the lambda expressions in Langmuir stuff to avoid conflicts with #889.

I propose a new tongue twister:

I pickled a Particle of pickled PEP8ers.

try:
self._pars = collections.defaultdict(lambda: None)
self._pars = collections.defaultdict(return_none)
Copy link
Member Author

Choose a reason for hiding this comment

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

One of the things I read suggested that defaultdict objects cannot be pickled, so we might need to venture away from them. Out of scope for this PR, though.

@codecov
Copy link

codecov bot commented Feb 15, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.67%. Comparing base (6f163e0) to head (95df9ce).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1013      +/-   ##
==========================================
+ Coverage   96.62%   96.67%   +0.04%     
==========================================
  Files          64       64              
  Lines        6249     6278      +29     
==========================================
+ Hits         6038     6069      +31     
+ Misses        211      209       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@StanczakDominik StanczakDominik left a comment

Choose a reason for hiding this comment

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

The changes are okay, but (see comment):

Comment on lines 52 to 53
* Avoid using `lambda`. Anonymous functions created with `lambda`
cannot be pickled, and `lambda` notation may be unfamiliar to
Copy link
Member

Choose a reason for hiding this comment

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

That lambdas cannot be pickled is of course true; but as far as I know, and "Is there an easy way to pickle a python function (or otherwise serialize its code)?" on Stack seems to confirm, Python functions just can't be pickled at all, lambda or no lambda. "How to pickle a python function with its dependencies?" seems to show you can do it using marshal or cloudpickle packages, but that's not exactly the same thing - I don't think scipy multiprocessing will handle that, for example.

@@ -162,8 +166,11 @@ def __init__(
)
set_abundances = False

def return_none():
return None
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem like that one's actually used - see codecov warning!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, cool — that means we can use a regular dict instead of a defaultdict when storing the parameters for IonizationStateCollection.

@namurphy
Copy link
Member Author

I revised the statement about avoiding the use of lambda in the code guide, and removed the reference to pickling. If this looks good, I'm happy with it being merged. Thanks for the quick and helpful review!

@StanczakDominik StanczakDominik merged commit e2b883b into PlasmaPy:master Feb 16, 2021
@StanczakDominik
Copy link
Member

Boom! :)

@StanczakDominik StanczakDominik added this to the 0.6.0 milestone Mar 12, 2021
@namurphy namurphy deleted the acetic-acid branch August 14, 2021 13:04
@namurphy namurphy added plasmapy.particles Related to the plasmapy.particles subpackage maintenance General updates to package infrastructure labels May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance General updates to package infrastructure plasmapy.particles Related to the plasmapy.particles subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants