Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enable pickling for LLVMDouble class #213

Merged
merged 8 commits into from
Feb 7, 2018
Merged

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Feb 6, 2018

Fixes #212

@bjodah
Copy link
Contributor

bjodah commented Feb 6, 2018

You're (insanely) fast @isuruf :) -- I was just going to start looking into this.
Changing load/dump to loads/dumps was on the top of my list but you're way ahead here.
The approach looks great to me. +1 to merge when tests pass.

@@ -4369,7 +4369,7 @@ cdef class _Lambdify(object):
cdef vector[int] accum_out_sizes
cdef object numpy_dtype

def __init__(self, args, *exprs, cppbool real=True, order='C', cppbool cse=False):
def __init__(self, args, *exprs, cppbool real=True, order='C', cppbool cse=False, cppbool load=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should name the kwarg _load to signal that end-users should be touching it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I'll change this

@@ -4639,6 +4661,8 @@ IF HAVE_SYMENGINE_LLVM:
addr2 = cast(<size_t>&self.lambda_double[0], c_void_p)
return create_low_level_callable(self, addr1, addr2)

def llvm_loading_func(*args):
return LLVMDouble(args, _load=True)
Copy link
Contributor

@bjodah bjodah Feb 6, 2018

Choose a reason for hiding this comment

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

Actually I think the _load kwarg could go into __cinit__ if we cdef the return value here:

def llvm_loading_func(*args):
    cdef LLVMDouble llvm_double = LLVMDouble(args, _load=True)
    return llvm_double

(untested)
cf. e.g. this

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow. Can you elaborate more? (Or a PR. 😃)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not important (actually not sure it matters much), so let's leave it for now (and I can try to make a PR against master after this is merged if I can find a way to hide the _load kwarg).

@bjodah
Copy link
Contributor

bjodah commented Feb 6, 2018

Somewhat unrelated: in SymEngine (the C++ library) all Basic instances are immutable and they have typeid, so I guess they serialize? I haven't found the function for that though, is there any such? (I'm thinking they should in principle serialize to bytestreams too?)

EDIT: I just saw #205, I guess that is the place for this discussion.

@isuruf
Copy link
Member Author

isuruf commented Feb 6, 2018

in SymEngine (the C++ library) all Basic instances are immutable and they have typeid, so I guess they serialize? I haven't found the function for that though, is there any such? (I'm thinking they should in principle serialize to bytestreams too?)

I know nothing about serialization in C++. Can you open an issue about it in the C++ repo, so that we can ask those who know about it?

@bjodah
Copy link
Contributor

bjodah commented Feb 6, 2018

I know nothing about serialization in C++. Can you open an issue about it in the C++ repo, so that we can ask those who know about it?

I opened symengine/symengine#1394

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants