esitarski / CrossMgr

Cyclo Cross Management Application
MIT License
41 stars 21 forks source link

CrossMgr arbitrary limits max laps to 200 #48

Closed mbuckaway closed 4 years ago

mbuckaway commented 4 years ago

crossmgrerror

Describe the bug CrossMgr ignores laps setup in the category tab if set higher than 200 and seems to have a hard coded limit of 200 laps.

To Reproduce Steps to reproduce the behavior:

  1. Setup a race to 450 laps (or anything more than 200)
  2. Run the race. Lap counter will start at the pre-programmed amount.
  3. After about two laps when chips start reading, the lap counter will drop to 200 or 198. There is nothing one can do about it. As the race runs, the record screen reports laps to go counting down from 200.
  4. When the record screen gets to zero, it keep counting the laps....this time counting up until the race is over. the lap counter reads zero and stays at zero. The race appears to score correctly.

Expected behavior Laps should count down from the number of laps set in category settings. It should never change the number laps.

Desktop (please complete the following information):

Additional context Add any other context about the problem here.

esitarski commented 4 years ago

Yes, entriesMax in class Rider has a limit of 200. This was the longest Track race I could find.

There has to be a limit, otherwise stray reads can confuse interpolation and you end up with 1,000s of generated lap times. I bumped it up to 500.

On Sun, Feb 2, 2020 at 9:34 AM Mark Buckaway notifications@github.com wrote:

Describe the bug CrossMgr ignores laps setup in the category tab if set higher than 200 and seems to have a hard coded limit of 200 laps.

To Reproduce Steps to reproduce the behavior:

  1. Setup a race to 450 laps (or anything more than 200)
  2. Run the race. Lap counter will start at the pre-programmed amount.
  3. After about two laps when chips start reading, the lap counter will drop to 200 or 198. There is nothing one can do about it. As the race runs, the record screen reports laps to go counting down from 200.
  4. When the record screen gets to zero, it keep counting the laps....this time counting up until the race is over. the lap counter reads zero and stays at zero. The race appears to score correctly.

Expected behavior Laps should count down from the number of laps set in category settings. It should never change the number laps.

Desktop (please complete the following information):

  • LInux Ubuntu 18.04
  • Version 3.0.46.beta

Additional context Add any other context about the problem here.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AABGXKNJIPOWS7FBFRUCYCLRA3KX7A5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IKNFFQQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGXKJHMCPVYB5B3K46Y4TRA3KX7ANCNFSM4KO2ADKQ .

--

Edward Sitarski

mbuckaway commented 4 years ago

No. No.No.No.No.No.No.No.No.

Max LAPS is set to 200. Not max entries. We have 10 riders on the track....not 200. The lap counter and other parts of the programs assume max 200 laps.

The lap counter should never change the max laps to anything other than what is configured in the category settings. This should NOT be a hard coded limit. Period. If the user screws this up, it their fault.

If I setup a race 10,000 laps, it should work.

esitarski commented 4 years ago

Yes yes yes yes yes ;) Max entries does not refer to the number of riders in the race. That is unlimited. maxEntries (perhaps misnamed) refers to the number of entries generated the interpolation for lap error correction. It was 200, now 500. That is the fix.

On Sun, Feb 2, 2020 at 4:18 PM Mark Buckaway notifications@github.com wrote:

No. No.No.No.No.No.No.No.No.

Max LAPS is set to 200. Not max entries. We have 10 riders on the track....not 200. The lap counter and other parts of the programs assume max 200 laps.

Please indicate where in the program this limit exist so it can be moved to a setting.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AABGXKIUIKEGGR7DJXBHAC3RA42CNA5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKSBHWY#issuecomment-581178331, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGXKLXK2WHKG3VU3Z2D4DRA42CNANCNFSM4KO2ADKQ .

--

Edward Sitarski

mbuckaway commented 4 years ago

No.

Get rid of the hard coded limits. Period. This is just bad programming practice.

Then max laps of a race should be whatever I set in the properties->categores. There should not be a hard coded limit.

We are running a 1001 lap race. I should be able to run a 10,000 lap race and it should not matter.

esitarski commented 4 years ago

Mark, there is more to it. Say there is a 60min race, and for some reason, the first two reads are 0.001s apart. Of course, this can't happen with CrossMgrImpinj, but, it could come from RaceResult (or JChip, or something else). Without a limit, CrossMgr will generate 60601000=36meg laps for the rider. When this happens (and it has), wxPython chokes trying to update the Results screen which has 36Meg columns in it. Of course, after a few laps everything gets sorted out. But, CrossMgr has already crashed.

I agree that a limit is not ideal. Do you have a better suggestion? Another way to handle this is to set the minPossibleLapTime (which defaults to 0.0). This would filter spurious reads, but, users can still set it to zero. The hard limit has proven to be more idiot proof.

On Sun, Feb 2, 2020 at 4:40 PM Mark Buckaway notifications@github.com wrote:

No.

Get rid of the hard coded limits. Period. This is just bad programming practice.

Then max laps of a race should be whatever I set in the properties->categores. There should not be a hard coded limit.

We are running a 1001 lap race. I should be able to run a 10,000 lap race and it should not matter.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AABGXKMOUUTA2ZK2XYULRD3RA44T5A5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKSBXRI#issuecomment-581180357, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGXKMFKWXHBCRMHFOKMXDRA44T5ANCNFSM4KO2ADKQ .

--

Edward Sitarski

mbuckaway commented 4 years ago

Turns out this is a hard coded item in Model.py. Some discussion needs to happen to figure out a proper solution.

stuartlynne commented 4 years ago

Probably min lap time should default to some small non zero value

On Sun, Feb 2, 2020 at 2:08 PM Edward Sitarski notifications@github.com wrote:

Mark, there is more to it. Say there is a 60min race, and for some reason, the first two reads are 0.001s apart. Of course, this can't happen with CrossMgrImpinj, but, it could come from RaceResult (or JChip, or something else). Without a limit, CrossMgr will generate 60601000=36meg laps for the rider. When this happens (and it has), wxPython chokes trying to update the Results screen which has 36Meg columns in it. Of course, after a few laps everything gets sorted out. But, CrossMgr has already crashed.

I agree that a limit is not ideal. Do you have a better suggestion? Another way to handle this is to set the minPossibleLapTime (which defaults to 0.0). This would filter spurious reads, but, users can still set it to zero. The hard limit has proven to be more idiot proof.

On Sun, Feb 2, 2020 at 4:40 PM Mark Buckaway notifications@github.com wrote:

No.

Get rid of the hard coded limits. Period. This is just bad programming practice.

Then max laps of a race should be whatever I set in the properties->categores. There should not be a hard coded limit.

We are running a 1001 lap race. I should be able to run a 10,000 lap race and it should not matter.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AABGXKMOUUTA2ZK2XYULRD3RA44T5A5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKSBXRI#issuecomment-581180357 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AABGXKMFKWXHBCRMHFOKMXDRA44T5ANCNFSM4KO2ADKQ

.

--

Edward Sitarski

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AACIUWP3TJ625MJ4DEJ7AGDRA4765A5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKSCLBA#issuecomment-581182852, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACIUWM5WYNVX7EWKS6C6CLRA4765ANCNFSM4KO2ADKQ .

-- Sent from Gmail Mobile

mbuckaway commented 4 years ago

Min lap time was set to 8 secs and not zero as I wanted to any reads less than 8 secs tossed.

The problem is the GUI assumes there is a need to view 100% of the data all the time. The entriesMax is a hack. The UI should be doing pagination and only displaying a subset of the data at any given time. For an example, search something like "cat" on Amazon. They page through the items allowing the user to select if there is 25, 50, 100 items per page, etc.. yes, 99% of the races never really go over 25 laps, but the application should be designed to handle 10,000, 100, 000 or whatever (within the limits of available memory).

Like I said, I would rather discuss this over the phone. I'm sure there are considerations we are both missing.

esitarski commented 4 years ago

Sure. The core problem is the number of columns in the Results screen, which can happen in the example I gave previously. The wxPython Grid fails when this is too big (or uses so much memory the program crashes). That was the original motivation for adding a max laps limit.

Yes, we can also hardcode the minPossibleLapTime to a min of 2sec. But, that's also a hardcode. Someone who wants to time sometime with laps shorter than 2sec would think that was unreasonable (similar to those trying to have a race with more than 200 laps today ;) This really is the first time this has come up.

I am still recovering from being volunteer manager at 2020 TISSOT Track World Cup and 2020 Para Cycling Track World Championships and will be available later this week for a call.

On Mon, Feb 3, 2020 at 12:31 PM Mark Buckaway notifications@github.com wrote:

Min lap time was set to 8 secs and not zero as I wanted to any reads less than 8 secs tossed.

The problem is the GUI assumes there is a need to view 100% of the data all the time. The entriesMax is a hack. The UI should be doing pagination and only displaying a subset of the data at any given time. For an example, search something like "cat" on Amazon. They page through the items allowing the user to select if there is 25, 50, 100 items per page, etc.. yes, 99% of the races never really go over 25 laps, but the application should be designed to handle 10,000, 100, 000 or whatever (within the limits of available memory).

Like I said, I would rather discuss this over the phone. I'm sure there are considerations we are both missing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AABGXKJEY7CEAGTDWZR7ZPLRBBIIBA5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKUWN7A#issuecomment-581527292, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGXKO33KZMLSJUVEFXI7TRBBIIBANCNFSM4KO2ADKQ .

--

Edward Sitarski

stuartlynne commented 4 years ago

I was suggesting default for min time, e.g. 2 or 4 seconds. But still could be set to 0 or something else manually.

On Mon, Feb 3, 2020 at 12:15 PM Edward Sitarski notifications@github.com wrote:

Sure. The core problem is the number of columns in the Results screen, which can happen in the example I gave previously. The wxPython Grid fails when this is too big (or uses so much memory the program crashes). That was the original motivation for adding a max laps limit.

Yes, we can also hardcode the minPossibleLapTime to a min of 2sec. But, that's also a hardcode. Someone who wants to time sometime with laps shorter than 2sec would think that was unreasonable (similar to those trying to have a race with more than 200 laps today ;) This really is the first time this has come up.

I am still recovering from being volunteer manager at 2020 TISSOT Track World Cup and 2020 Para Cycling Track World Championships and will be available later this week for a call.

On Mon, Feb 3, 2020 at 12:31 PM Mark Buckaway notifications@github.com wrote:

Min lap time was set to 8 secs and not zero as I wanted to any reads less than 8 secs tossed.

The problem is the GUI assumes there is a need to view 100% of the data all the time. The entriesMax is a hack. The UI should be doing pagination and only displaying a subset of the data at any given time. For an example, search something like "cat" on Amazon. They page through the items allowing the user to select if there is 25, 50, 100 items per page, etc.. yes, 99% of the races never really go over 25 laps, but the application should be designed to handle 10,000, 100, 000 or whatever (within the limits of available memory).

Like I said, I would rather discuss this over the phone. I'm sure there are considerations we are both missing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AABGXKJEY7CEAGTDWZR7ZPLRBBIIBA5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKUWN7A#issuecomment-581527292 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AABGXKO33KZMLSJUVEFXI7TRBBIIBANCNFSM4KO2ADKQ

.

--

Edward Sitarski

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esitarski/CrossMgr/issues/48?email_source=notifications&email_token=AACIUWLNF7HFH24S3HUYWK3RBB3O5A5CNFSM4KO2ADK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKVHU6Y#issuecomment-581597819, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACIUWMVYXH4YA7ADIHZ633RBB3O5ANCNFSM4KO2ADKQ .

-- __O____ -\<,____ ____()/()___


Stuart_Lynnestuart.lynne@gmail.com604-518-1749(m)__604-461-7532(h)

esitarski commented 4 years ago

I changed the max number of allowed time entries 1000 (instead of 200). This is a backstop to avoid the Grid in the Results (and Passings) screen from crashing from too many columns. Also, the MinPossibleLapTime now defaults to 2.0s. This should address bounce rfid reads which can cause an explosion of the number of projected laps in a timed race.

Better, but not 100% solution: CrossMgr still has an upper limit of 1000 laps.

A more general fix would involve changing the Results and Passings screens so they limit the number of columns shown. It would not surprise me to find other performance issues with huge lap counts. After all, CrossMgr started from Cyclocross races.

mbuckaway commented 4 years ago

What happens when one runs a race simulation with 1000 laps? It should be easy enough to generate a fake impinj chip file with 20 riders.

CrossMgr is no longer just for cross races (and I figure for a 24hr CX race, it would be 184 laps anyways).

esitarski commented 4 years ago

Max laps is now set to 2000. Min possible lap time now defaults to 2.0 secs.

I did simulations of 2000 laps for 1000 riders. Unlike previous versions, the latest wxPython Grid handled it without crashing, although performance was sluggish with less than 8Gig memory. The Chart also didn't scroll well initially. This was addressed by adding a binary search to find the start of the first time to display in the window rather than linear skipping. All other displays seemed to be OK.

Fortunately, the web page already has a feature to display laps up to every 100. It also attempts to find a reasonable default based on the lap count in the data. This results in rendering a 20 column table of lap times for 2000 laps (of course, the full data is still present). The Chart and SItuations displays don't crash the browser (fortunately they are canvas based, not svg based), but they aren't all that useful either. Not sure there is a better visualization alternative for large lap counts.

Removing max laps altogether is a bad idea. If a user set Min possible lap time to 0.0 and used another RFID reader other than CrossMgrImpinj, it is possible to get bad data into CrossMgr that will blow up lap prediction and crash. If CrossMgr doesn't crash you can fix the bad data.