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 TransformBlock and DipoleBlock objects #27

Closed
wants to merge 9 commits into from

Conversation

alisterburt
Copy link
Collaborator

This PR adds

  • a DipoleBlock object for simple definition of oriented particles
  • a TransformBlock object which can store and apply 3D transformations onto existing ParticleBlocks

and closes #14

It seems strange that TransformBlock inherits from ParticleBlock and has to have properties, what do you think about adding an OrientedPointBlock which would be a parent class to both of these?

Copy link
Owner

@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

Now that I look at this code, I am a bit confused as to what DipoleBlock, OrientedPointBlock and TransformBlock bring to the table. Arent all of these effectively just different ways of describing the same concept as ParticleBlock?
Particles are just Dipoles/OrientedPoints with an additional Properties block... Why not use particles to describe Dipoles, or viceversa? And with our new Achemists, now even TransformBlock can simply be an Alchemist that takes 2 particle-like objects and returns another ParticleBlock...
I think that if we keep this concept abstract, we can avoid a lot of code duplication and confusing classes.

However, it makes sense to have a generic particles/dipoles/orientedpoint block without properties, and make ParticleBlock a daughter class of that.

peepingtom/core/datablocks/dipoleblock.py Outdated Show resolved Hide resolved
peepingtom/core/datablocks/dipoleblock.py Outdated Show resolved Hide resolved
@alisterburt
Copy link
Collaborator Author

I agree with some but not all of what you've said, specifically...

  • DipoleBlock is fundamentally different from an OrientedPointBlock in that it contains only start and endpoints. These are not enough to unambiguously define an orientation, which requires definition of the new basis (e.g. any aligning z of a particle to a dipole you have an infinite number of possible orientations by rotating the particle around its new z axis z). Additionally, the DipoleBlock should be able to be used to align specific vectors in other reference frames to specific vectors in the reference frame of the DipoleBlock... I'm not being clear hear but we can discuss properly tomorrow

  • on ParticleBlock and TransformBlock being the same thiing: yes and no. You're right in that the data looks the same, positions and orientations. I remember we discussed this and agreed that the notion of applying particles onto particles didn't really make sense, whereas applying transforms onto particles seems natural. I can imagine that an additional step will often be required to generate a TransformBlock from a ParticleBlock anyway, because the ParticleBlock will be generated in the reference frame of a reconstruction (subparticles with respect to the center of the reconstruction) whereas the TransformBlock needs to be defined relative to the center of that reconstruction... it feels like keeping the object and concepts different is useful in situations like this

Looking forward to hearing your thoughts!

@brisvag
Copy link
Owner

brisvag commented Nov 22, 2020

I don't think it's a good idea to add big files like these example data files to the repo. I would prefer to host them separately, so we don't bog down this repo with heavy stuff.

@brisvag
Copy link
Owner

brisvag commented Nov 23, 2020

As we discussed, we will make a repo with example data. We need to be careful to remove these files from your branch's history, or we will still carry them around forever.

@brisvag
Copy link
Owner

brisvag commented Nov 23, 2020

#34 supersedes this. Closing.

@brisvag brisvag closed this Nov 23, 2020
@alisterburt alisterburt deleted the TransformBlock branch March 12, 2021 16:42
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