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

Update sol save/load; add test #198

Merged
merged 9 commits into from
Mar 2, 2023
Merged

Update sol save/load; add test #198

merged 9 commits into from
Mar 2, 2023

Conversation

jeffjennings
Copy link
Collaborator

@jeffjennings jeffjennings commented Mar 1, 2023

I ran into an issue saving/loading a sol object from a debris fit -- pickle was struggling to interpret sol._vis_map._scale_height as a function:

saving: AttributeError: Can't pickle local object '<some function for H(R)>'
loading: OSError: Failed to interpret file '<some path>_frank_sol.obj' as a pickle

Replacing pickle with dill resolves this. PR changes:

  • io.py: Updates pickle calls to dill
  • tests.py: Adds test for save/load of a sol from a standard and debris fit

@rbooth200
Copy link
Collaborator

rbooth200 commented Mar 1, 2023

So _scale_height is a function (or class object), but it's probably ok to remove it from the visibility mapping object. I'd just store the result of scale_height(self.r) as _scale_height instead of the function itself.

That said, would this issue still arise if you used a class / function to specify the scale-height in fit.py instead of eval-ing a user argument? The reason I ask this is because eval is a security vulnerability so we should not use it with general user input.

@jeffjennings
Copy link
Collaborator Author

Ah ok I see. I was using eval in order to parse a scale-height function from the parameter file (at one point you suggested scale-height should be a function in the .json). Is it just enough for practical purposes to make scale_height in the .json a float that goes into an H(R) function in fit.py as scale_height * R?

I've reverted dill to pickle and changed self._scale_height to scale_height(self.r) in VisibilityMapping.

@rbooth200
Copy link
Collaborator

I'd probably choosing something slightly more flexible, e.g. a power law with a cut off. Maybe:H(r) = h0 r**a exp (-(r/r0)**b) but I don't really mind. We provide complete flexibility though the library version anyway.

@jeffjennings
Copy link
Collaborator Author

Ok I implemented the power law in #195. This PR has very minor changes then, ready for review.

@jeffjennings jeffjennings requested a review from rbooth200 March 2, 2023 04:11
Copy link
Collaborator

@rbooth200 rbooth200 left a comment

Choose a reason for hiding this comment

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

Can you add back the save/load test? It will catch problems with pickle should they arise again.

@jeffjennings
Copy link
Collaborator Author

Sure, done in 3e79f22

@rbooth200 rbooth200 self-requested a review March 2, 2023 17:55
@rbooth200 rbooth200 merged commit c83291d into master Mar 2, 2023
@jeffjennings jeffjennings deleted the pickle_to_dill branch March 2, 2023 18:11
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.

2 participants