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

Corrected overflow with ele_stop <= ele_start #440

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

freddieknets
Copy link
Contributor

Description

Using ele_stop in track() implies that the last turn will be incomplete (unless ele_stop=0); hence no extra turns should be added.

I.e. if we do track(num_turns=2, ele_start=10, ele_stop=20) the first turn tracks from element 10 to the end, and the second turn from the start until element 20 (and similarly for e.g. num_turns=3 where the middle turn is a complete one).

In this case, nothing changes when ele_stop <= ele_start:
If we do track(num_turns=2, ele_start=20, ele_stop=10) the first turn tracks from element 20 to the end, and the second turn from the start until element 10.

There is one important exception: when ele_stop <= ele_start and num_turns=1. This is in principle impossible, but writing track(ele_start=20, ele_stop=10) should be a convenient way to track only a part of the line (that passes through the origin) without having to worry about increasing num_turns manually. Hence, for this very specific overflow case, and only for this one, the tracker changes num_turns to 2.

This was the original idea, however, the overflow was applied to all values of num_turns which creates a discrepancy in physical interpretation between the cases ele_stop <= ele_start and ele_start < ele_stop.

Checklist

Mandatory:

  • I have added tests to cover my changes
  • All the tests are passing, including my new ones
  • I described my changes in this PR description

Optional:

  • The code I wrote follows good style practices (see PEP 8 and PEP 20).
  • I have updated the docs in relation to my changes, if applicable
  • I have tested also GPU contexts

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.

1 participant