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

Bugfix: Don't use symmetries for weighting potentials #216

Merged
merged 3 commits into from
Sep 14, 2021
Merged

Conversation

lmh91
Copy link
Collaborator

@lmh91 lmh91 commented Sep 13, 2021

Crucial bugfix for the calculation of the weighting potentials.

Bug: The grid for the weighting potential were initialized with the symmetries of the world which are only valid for the
electric potential and not the individual contacts.

Copy link
Collaborator

@fhagemann fhagemann left a comment

Choose a reason for hiding this comment

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

The tests still fail. The tolerance needs to be adjusted or the Event needs to be modified.

Copy link
Collaborator

@fhagemann fhagemann left a comment

Choose a reason for hiding this comment

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

We are ignoring the keyword full_2π here. Do we want this?


if for_weighting_potential && world_Δφ > 0
world_φ_int = SSDInterval{T, :closed, :open, :periodic, :periodic}(0, 2π)
world_Δφ = width(world_φ_int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we always want it to be full ? I think this makes the full_2π keyword obsolete?

We could set full_2π = true here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think it is obsolete. It's nowhere used. I will remove it.

I would keep it as we did it before. Always calculate the weigting potentials for 2π.
Only if it is completly phi-symmetric. Than, also only 2D.

I think we should refactor the symmetry handling, #215, in general. But for now we just need a quick fix.

if full_2π || (for_weighting_potential && (world.intervals[2].left != world.intervals[2].right))
L, R, BL, BR = get_boundary_types(world_φ_int)
int_φ = Interval{L, R, T}(endpoints(world_φ_int)...)
if full_2π || (for_weighting_potential && (world_φ_int.left != world_φ_int.right))
Copy link
Collaborator

Choose a reason for hiding this comment

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

.. and then we can check only the statement if full_2π here?

Copy link
Collaborator

@fhagemann fhagemann left a comment

Choose a reason for hiding this comment

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

Nice!

@lmh91 lmh91 merged commit 5564a9c into master Sep 14, 2021
@fhagemann
Copy link
Collaborator

fhagemann commented Sep 14, 2021

I am a bit surprised that the tests for the segBEGe detector did not fail (@test signalsum == 2) if the weighting potentials were not calculated the right way and what we can do tr avoid tests passing when things like these happen again.

@lmh91
Copy link
Collaborator Author

lmh91 commented Sep 14, 2021

I am a bit surprised that the tests for the segBEGe detector did not fail (@test signalsum == 2) if the weighting potentials were not calculated the right way and what we can do tr avoid tests passing when things like these happen again.

Because contact 1 lied (at least partially) inside the segment of the world which was covered by the symmetry defined for the electric potential. And also the event was spawned inside this volume. So a sum of 2 was produced. However, the pulse shape is probably also wrong.

@fhagemann fhagemann deleted the wp_bugfix branch October 11, 2021 08:47
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.

None yet

2 participants