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

Do calculations as float and not as int for plotting points. #288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tzachi-dar
Copy link
Collaborator

This fixes a problem where points some times seemed too close to each other.

Signed-off-by: Tzachi Dar [email protected]

The current code moves from seconds to units of 2.5 minutes.
As a result, since due to measuring errors two points are not guaranteed to be in 5 minutes apart, they might end much closer than they should be.

I'll upload pictures tomorrow for the change.

This fixes a problem where points some times seemed too close to each other.

Signed-off-by: Tzachi Dar <[email protected]>
@AdrianLxM
Copy link
Collaborator

Isn't the whole point of FUZZER to floor the values to timeslots and make them equally distanced most of the time even if the bt connection takes a few seconds more (with xBridge e.g.)?

@mgranberry, as far as I remember you introduced this?

Like it would be after a merge of this pr: This behaviour would not be in, but there would be a lot of legacy code around it :/

Wouldn't it also introduce timings with the precision of milliseconds? In case we want to keep the fuzzer in but weaken the effect we could set it to minutes (or less)?

How do we make them equidistant on the chart - without too much calculation costs?
Like it is today it worked most of the time - but on some rarer occasions when some consecutive values were around the multiple of 2.5 we introduced an even larger gap.

@tzachi-dar
Copy link
Collaborator Author

Will look for @mgranberry answer, but in the meantime...

if the readings are a few seconds apart, than I don't think it has an
effect on the picture one sees.

Today, the code breaks them in parts of 2.5 minutes, but this 2.5 minutes
is not related to when the transmitter is actually transmitting. It could
be that one point is at 08:00:00 and the other is at 08:04:59 and they will
move 2.5 minutes from one another.

This can be fixed, probably by going over the points once, looking for
their modulo 300, time, in order to get an understanding of where the
transmitter is really transmitting, and then do a2.5 minutes rounding to
this point, (than).
I can try and see how long this will be taking to run such code...

Adrian, can you explain more about the legacy code issues that you see?

Thanks
Tzachi

On Thu, Mar 3, 2016 at 10:55 AM, AdrianLxM [email protected] wrote:

Isn't the whole point of FUZZER to floor the values to timeslots and make
them equally distanced most of the time even if the bt connection takes a
few seconds more (with xBridge e.g.)?

@mgranberry https://github.com/mgranberry, as far as I remember you
introduced this?

Like it would be after a merge of this pr: This behaviour would not be in,
but there would be a lot of legacy code around it :/

Wouldn't it also introduce timings with the precision of milliseconds? In
case we want to keep the fuzzer in but weaken the effect we could set it to
minutes (or less)?

How do we make them equidistant on the chart - without too much
calculation costs?
Like it is today it worked most of the time - but on some rarer occasions
when some consecutive values were around the multiple of 2.5 we introduced
an even larger gap.


Reply to this email directly or view it on GitHub
#288 (comment)
.

@AdrianLxM
Copy link
Collaborator

Adrian, can you explain more about the legacy code issues that you see?

If we would get rid of the fuzzer behaviour alltogether, I would have eliminated the fuzzer... but I've just seen, that PointValue just accepts float values, so we have to make the timestamps (long) float anyways.

@jstevensog
Copy link
Collaborator

Personally, I've never been a fan of fuzzer. Until I figured out what it did, it made debugging what was going on with xBridge2 difficult. I had to remove it in a branch to get around it.
Cheers

---- AdrianLxM wrote ----

Adrian, can you explain more about the legacy code issues that you see?

If we would get rid of the fuzzer behaviour alltogether, I would have eliminated the fuzzer... but I've just seen, that PointValue just accepts float values, so we have to make the timestamps (long) float anyways.


Reply to this email directly or view it on GitHub.

@tzachi-dar
Copy link
Collaborator Author

Well, It seems the fuzzer was added around the date 22.3.2015.
There have been huge merges on that time, so it is hard to tell what was the main reason for it.

The way I understand this, there is no problem with the fuzzer, but with the fact that it does not work well.
Basicly, what it does is group packets in units of 2.5 minutes together.
But since the 2.5 minutes barrier is arbitrary, there are times that it does not work well.
practically speaking the buckets are in the following places (all in seconds):

0, 150, 300, 450, 600, 750, 900.

So, first (but not really related) I believe that before doing the calculations, we should add 75 to the value before doing the division. This actually means that we should use the function round() and not flour()...

Next step is go over all the packets and find the relative offset.
This offset will be removed from the points before doing the calculations.

Here is the way that I suggest to do the calculations of the offset:
int sum = 0;
for(point : points) {
sum += points.time / 150.
}
offset = sum / points.size();

and the point will be set to
xAxis = (point.time + 75 - offset)/150; // all calculations are in int.

Here is an example of how this will look with some numbers.
Let's assume that our readings are on:
20, 320, 640, 971, 1199, 1520

Basicly, this are the points with offset == 20 and one point (1199) has shifted. It should have been 1220.

So, applying the formula, (point.time + 75 - offset) on this points will yeld (offset == 20):
75, 375, 695, 1026, 1254, 1575

This points when divided by 150, will give us good values (in the middle of the buckets).

I hope I was clear, let me know if not, and please let me know if this sounds like a good plan.

Obviously, all this was written with the assumption that calculation the offset will be fast. I'll measure that tomorrow, and see how long it will take. (two points about this, (1) I believe that we can calculate the offset based on smaller number of points (for example 20). (2) we can also cache this value all together).

All input is welcomed.
Tzachi

@AdrianLxM
Copy link
Collaborator

round versus floor:
If we use round or floor doesn't really matter for the chart. Each has a boarder case where the values start to jump. Adding the 75 seconds will fit better with the xAxis-Lables so I like it :)

the timeframe/buckets:
As the values come in about every 5 minutes, the bucket-offset (distance of bucket centres) should be a divider of 5 minutes. The 150 seconds is ok imho.

the offset:

int sum = 0; for(point : points) { sum += points.time / 150. } offset = sum / points.size();

this would basically make the offset the average time (divided by 150, already floored?).
Does that work if the number of points is even?
Does that work if there are points missing (gaps)?

Also I think there is a simpler solution without the need to loop over all values twice:
As we are just handling values of one day, the drift will not be large. We could simply take the time of the first value as centre of the first bucket. (that would be the offset in @tzachi-dar's example?) If values then come a few seconds earlier or later they still fluctuate around the centre of the bucket and not around the edge what causes the problems now.

I have never seen a case where one value was off more than 30 seconds (xBridge doesn't send it in such a case e.g.). So even in a worstcase scenario it would still make for a good offset that defines the centres of the buckets/timeframes.

In short: I would simply take the time of the first value as offset.

@mgranberry
Copy link
Collaborator

The fuzzer was Stephen's addition. I only cleaned it up a little when fixing something for the Share2. I never really liked it because it seems unwise to throw away any information from the devices.

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