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

Add structref as a mutable struct that is pass-by-ref #5978

Merged
merged 19 commits into from Jul 15, 2020

Conversation

sklam
Copy link
Member

@sklam sklam commented Jul 10, 2020

Closes #5397

Add structref to support mutable structure that can be extended like typical custom types (i.e. with @overload and friends). Also, support some nice default to minimizing work; i.e. default boxer/unboxer, default getter/setter, default constructor.

TODOs:

  • add docs. to be included in a different PR.

@stuartarchibald stuartarchibald self-assigned this Jul 10, 2020
@sklam sklam marked this pull request as ready for review July 13, 2020 21:19
@sklam sklam changed the title Add structref to as a mutable struct that is pass-by-ref Add structref as a mutable struct that is pass-by-ref Jul 14, 2020
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, it's nice to see this implemented, think it will prove very useful. RE review, just a few, mostly minor, things to resolve.

numba/core/structref.py Outdated Show resolved Hide resolved
numba/core/structref.py Outdated Show resolved Hide resolved
numba/core/types/containers.py Outdated Show resolved Hide resolved
numba/core/types/containers.py Outdated Show resolved Hide resolved
numba/core/types/containers.py Outdated Show resolved Hide resolved
Comment on lines 109 to 125
@lower_setattr_generic(struct_typeclass)
def struct_setattr_impl(context, builder, sig, args, attr):
[inst_type, val_type] = sig.args
[instance, val] = args
utils = _Utils(context, builder, inst_type)
dataval = utils.get_data_struct(instance)
# cast val to the correct type
field_type = inst_type.field_dict[attr]
casted = context.cast(builder, val, val_type, field_type)
# read old
old_value = getattr(dataval, attr)
# incref new value
context.nrt.incref(builder, val_type, casted)
# decref old value (must be last in case new value is old value)
context.nrt.decref(builder, val_type, old_value)
# write new
setattr(dataval, attr, casted)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that anything can be done about it right now, but implementing the setattr as direct lowering means that if the type of the attribute and the type of the value mismatch it'll end up as a lowering error via a (correctly raising) unimplement cast.

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 thought that would happen too. But, it turns out that the typer for setattr is forcing the RHS to match the attribute type.

Comment on lines 31 to 72
class MyStruct(structref.StructRefProxy):

def __new__(cls, values, counter):
# Define this method to customize the constructor.
# The default takes `*args`. Customizing allow the use of keyword-arg.
# The impl of the method calls `StructRefProxy.__new__`
return structref.StructRefProxy.__new__(cls, values, counter)

# The below defines wrappers for attributes and methods manually

@property
def values(self):
return get_values(self)

@values.setter
def values(self, val):
return set_values(self, val)

@property
def counter(self):
return get_counter(self)

def testme(self, arg):
return self.values * arg + self.counter


@structref.register
class MyStructType(types.StructRef):
"""Test associated with this type represent the higher-level uses of
structef.
"""
pass


# Call to define_proxy is needed to register the use of `MyStruct` as a
# PyObject proxy for creating a Numba-allocated structref.
# The `MyStruct` class and then be used in both jit-code and interpreted-code.
structref.define_proxy(
MyStruct,
MyStructType,
['values', 'counter'],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there benefit in making this into a factory? Also wonder if the getters and setters like those here https://github.com/numba/numba/pull/5978/files#diff-9b2fbc1beb945aef47f6c71220bcd54dR101-R113 could be autogenerated in such a case, perhaps into a base class from which the other's can inherit and later be proxied? Perhaps not for now :)!

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 was trying to autogenerate the getters/setters via a base class. Just too much work for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree!

# mutate
td['a'].values += 10
self.assertEqual(td['a'].values, 12) # changed
self.assertEqual(td['a'].counter, 3.3) # unchanged
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps also check a td['b'] = MyStruct(4, 5.6) works, as I think it should? Makes me wonder, should a MyStruct(int8(4), 5.6) be castable, I'd guess not at the typing layer?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not casting the fields at the moment. That would be another potential enhancement

numba/tests/test_struct_ref.py Show resolved Hide resolved
docs/source/developer/repomap.rst Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 15, 2020
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
msg = "expecting a str for field name"
raise ValueError(msg)
if not isinstance(typ, Type):
msg = f"expecting a Numba Type for field type"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg = f"expecting a Numba Type for field type"
msg = "expecting a Numba Type for field type"

@stuartarchibald
Copy link
Contributor

Thanks for the fixes, I think the only remaining this is the f-string problem here #5978 (comment) else this looks good to go.

@stuartarchibald stuartarchibald added the 4 - Waiting on CI Review etc done, waiting for CI to finish label Jul 15, 2020
@sklam sklam removed the 4 - Waiting on author Waiting for author to respond to review label Jul 15, 2020
@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jul 15, 2020
@sklam sklam mentioned this pull request Jul 15, 2020
# Conflicts:
#	numba/core/types/containers.py
@sklam sklam merged commit 0596643 into numba:master Jul 15, 2020
@sklam sklam deleted the enh/mut_struct branch July 15, 2020 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to change structure of a custom object in place?
2 participants