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

Dynamic properties for ParticleSlice #1160

Merged
merged 5 commits into from
May 11, 2017
Merged

Dynamic properties for ParticleSlice #1160

merged 5 commits into from
May 11, 2017

Conversation

mkuron
Copy link
Member

@mkuron mkuron commented May 8, 2017

Fixes #958.

@RudolfWeeber @fweik @hmenke

Any properties that are not defined for ParticleSlice are automatically drawn from ParticleHandle, including docstrings. To the setter, one can either provide one value that is set on all particles or a list of values where the order corresponds to that of the slice members.

This turned out to be surprisingly difficult:

  • One cannot use setattr to add stuff to Cython classes, so I ended up wrapping the Cython class into a Python class that has all the autogenerated properties.
  • The wrapping cannot be done in the loop I added to the end of particle_data.pyx. I had to introduce a class that holds the attribute name as a member. If I generated the functions directly in the loop (i.e. capturing the loop variable), the attribute name a setter/getter was operating on would vary in a non-deterministic fashion (seems like some kind of reference counting bug in Python itself...).

Further improvements made along the way:

  • Fix ParticleSlice.__str__, which previously didn't return anything as int is not the same as numpy.int64.
  • Improve ParticleSlice.__str__ and ParticleList.__str__ so that the representations look like Numpy arrays or Python lists. Previously there were newlines and the name was missing.
  • Improve utils.check_type_or_throw_except so that it also allows numpy.int64 where int is requested.

PR Checklist

  • Tests?
    • Interface
    • Core

mkuron added 3 commits May 8, 2017 12:01
- Fix unittest without EXTERNAL_FORCES
- Rename ParticleSliceImpl to _ParticleSliceImpl
- Better unit tests
- Fix for scalar quantities
- Remove unneeded custom getters/setters
- Fix getter for slice that doesn't start at 0
Copy link
Member

@KaiSzuttor KaiSzuttor left a comment

Choose a reason for hiding this comment

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

Maybe add some more in-code documentation for future developers.

@mkuron
Copy link
Member Author

mkuron commented May 9, 2017

@KaiSzuttor, I'm currently refactoring _InjectParticleProperty to make it easier to understand. That should address your documentation request.

return res[:-1]
res += str(pl[i]) + ", "
# Remove final comma
return "ParticleSlice([" + res[:-2] + "])"
Copy link
Member

Choose a reason for hiding this comment

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

No need for this awkward comma removal

pl = ParticleList()
return "ParticleSlice([" + ", ".join(str(pl[i]) for i in self.id_selection if pl.exists(i)) + "])"

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 nicer, and it's more in line with how Python and Numpy print their own data types. Also, if I remove that space at the end, why not remove the trailing comma along with it.

return res[:-1]
res += str(self[i]) + ", "
# Remove final comma
return "ParticleList([" + res[:-2] + "])"
Copy link
Member

Choose a reason for hiding this comment

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

No need for this awkward comma removal

return "ParticleList([" + ", ".join(str(self[i]) for i in range(max_seen_particle + 1) if self.exists(i)) + "])"

@mkuron
Copy link
Member Author

mkuron commented May 9, 2017

I was able to get rid of the injector class by switching to functools.partial. It seems like the following two are different here, even though they look like they do the same:

functools.partial(geta, attribute=attribute_name)
lambda s: geta(s, attribute_name)

Explicitly capturing the loop variable like this

def geta(particle_slice, attribute=attribute_name):

didn't help either, even though that is the standard solution for this problem in pure Python. I really don't understand what is going on here, might be a Cython bug.

Anyway, once Travis passes, I think this can be merged.

@mkuron
Copy link
Member Author

mkuron commented May 11, 2017

Small change to the getter so it writes straight to a numpy array instead of converting a list into one at the end. Also extend unit test so it covers all code paths.

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.

4 participants