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

Fix quaternion default initialisation #300

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cniethammer
Copy link
Contributor

@cniethammer cniethammer commented Mar 20, 2024

This PR reverts changes to the defaults in the constructor made in 20a6b8f back to 0 + 0i + 0j + 0k (Q(w=0, x=0, y=0, z=0)) to address #291. The issue was first mentioned in #255

This has been tested using

  • internal unit tests
  • running examples with run-examples.sh adding the ./Generators/cubic_grid_generator/config.xml to the list of tests

The current initialization is neither the identity quaternion
1 + 0i + 0j + 0k (Q(w = 1, x = 0, y = 0, z = 0)) nor represents a pure rotation
as it is not normalized. Therefore revert changes made earlier
returning to Q(0,0,0,0).

Note: Initialisation to Q(0,0,0,0) is beneficial for performance reasons
to Q(1,0,0,0).

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer cniethammer self-assigned this Mar 20, 2024
@cniethammer cniethammer added the WIP Work In Progress label Mar 20, 2024
@cniethammer cniethammer force-pushed the fix-quaternion-default-initalisation branch from b5128cf to e11f7f1 Compare March 20, 2024 21:28
@cniethammer
Copy link
Contributor Author

Still failing the internalGenerator testcase - will need another look at this

@cniethammer cniethammer force-pushed the fix-quaternion-default-initalisation branch from e11f7f1 to 4fe755b Compare March 20, 2024 22:22
@cniethammer cniethammer removed the WIP Work In Progress label Mar 21, 2024
@cniethammer
Copy link
Contributor Author

@HomesGH Tests passing and ready for review.

@HomesGH
Copy link
Contributor

HomesGH commented Mar 21, 2024

To be honest, I still don't see the benefit of changing it to the invalid Q(0,0,0,0). Is it just performance-wise why we don't choose the identity Q(1,0,0,0)? There is much worse performing code, eg. the check for duplicated IDs.

For me, it would be ok to initialize the Quaternion class itself with 0,0,0,0, but all molecules should always carry valid data (Q(0,0,0,0) is invalid). Therefore, I see two options:

  1. Use the identity quaternion for initialization
  2. Use no defaults for the molecule class. All parameters have to be set by other classes/methods/etc. This might not be in line with the paradigm "Convention over configuration". It might also cause further trouble, see my comment below.

Furthermore, I can not guarantee that there are no hidden molecule initializations somewhere in the code. Those could break when using invalid data for initialization.

@HomesGH
Copy link
Contributor

HomesGH commented Mar 21, 2024

These lines could also cause trouble, even though, they should not be relevant in theory.

Also the PerCellGenerator misses the explicit passing of the quaternions.
Same here:
ParticleCellBase.cpp:154
DensityControl.cpp:374
PerCellGenerator.cpp:123

I can not guarantee that I found all Molecule/FullMolecule inits that use some of the defaults.

Remove the default parameters for the Molecule constructor to prevent
unintentional incorrect initialization of molecules. Fix all related
places in the code with the appropriate initialization values.

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer
Copy link
Contributor Author

I address the concern about missing initialization values by changing the Molecule constructor to require all parameters to be passed - removing default values from the interface. This should catch all places during compile time!

@HomesGH
Copy link
Contributor

HomesGH commented Apr 19, 2024

I address the concern about missing initialization values by changing the Molecule constructor to require all parameters to be passed - removing default values from the interface. This should catch all places during compile time!

This is ok-ish for me. Even though, always having to set all of those zeros including a single 1 looks a bit error-prone to me. E.g. accidentally setting vz to 1 instead of qw is simple:
Molecule m(id, comp, rx, ry, rz, 0., 0., 0., 1., 0., 0., 0., 0., 0., 0.);
versus
Molecule m(id, comp, rx, ry, rz, 0., 0., 1., 0., 0., 0., 0., 0., 0., 0.);

@@ -60,9 +60,9 @@ void ParticleDataFull::MoleculeToParticleData(ParticleDataFull &particleStruct,
particleStruct.D[2] = molecule.D(2);
}

void ParticleDataFull::ParticleDataToMolecule(const ParticleDataFull &particleStruct, Molecule &molecule) {
Molecule ParticleDataFull::ParticleDataToMolecule(const ParticleDataFull &particleStruct) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest: Is the whole object returned or just a pointer?
If former, isn't this quite bad since a Molecule object is rather big?

@HomesGH HomesGH changed the title Fix quaternion default initalisation Fix quaternion default initialisation May 22, 2024
@HomesGH HomesGH linked an issue Jun 5, 2024 that may be closed by this pull request
@HomesGH HomesGH added the bug Something isn't working label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix weird Quaternion default initialisation
2 participants