FAI-CIVL / FAI-Airscore

AirScore - online paragliding / hanggliding GAP-based scoring software.
https://airscore.cc/
GNU General Public License v3.0
13 stars 17 forks source link

error on scoring #203

Closed kuaka closed 3 years ago

kuaka commented 3 years ago

irscore_prod | File "/app/airscore/core/task.py", line 694, in create_json_elements airscore_prod | stats = {x: getattr(self, x) for x in TaskResult.stats_list if x in dir(self)} airscore_prod | File "/app/airscore/core/task.py", line 694, in <dictcomp> airscore_prod | stats = {x: getattr(self, x) for x in TaskResult.stats_list if x in dir(self)} airscore_prod | File "/app/airscore/core/task.py", line 483, in max_ss_time airscore_prod | return max(p.SSS_time for p in self.valid_results if p.SSS_time and p.SSS_time > 0) airscore_prod | ValueError: max() arg is an empty sequence this error can happen. I am not sure why but there is no feedback to user. Related to #140

biuti commented 3 years ago

I don't know how that could happen as there are checks that should avoid, anyway I changed the way those properties are working. I think those kind of errors shouldn't happen. (https://github.com/FAI-CIVL/FAI-Airscore/commit/763134a2c713d7620466cb1fced62e4462bbd9b0)

I also created some more unit test about scoring committed in https://github.com/FAI-CIVL/FAI-Airscore/commit/fac2244b796523785bb57ad6c2d4f4df9369a6b1

kuaka commented 3 years ago

still has issues. airscore_prod | File "/app/airscore/core/formulas/libs/leadcoeff.py", line 63, in tot_lc_calc airscore_prod | if res.real_start_time > t.min_dept_time: airscore_prod | TypeError: '>' not supported between instances of 'int' and 'NoneType'

the task in question is elapsed time, which may have not been tested since there were big changes.

any testing of scoring needs to consider race to goal, elapsed time, multi gate, difficulty calc, lead calcs no pilots in goal, all pilots in goal, some pilots in goal, some<min distance etc etc (and all combinations of these.

biuti commented 3 years ago

the problem here is that in task table type is lowercase, in taskObject view is uppercase. This is confusing. I don't remember the reason why it is like this, but we can either change taskObject view to be consistent with the table, or just add a .lower or .upper where task_type is used. I would prefer to adjust the view, as it has not too much sense to be different from the value in main table. Let me know.

kuaka commented 3 years ago

I don't follow as I thought task type was an enum and therefore consistent.

biuti commented 3 years ago

in view is upper task type

biuti commented 3 years ago

As it is just a view, changing it and adjusting the code to lower the value so older db version has no problem seems the thing to do.

kuaka commented 3 years ago

I too do not understand why a view is modifying the case, particularly for an enum. This is not a great idea. I know in the past the enum had been changed from "elapsed_time" to "elapsed time" so I suspect that it was changed for the front end to be more readable - this should happen in code not in the DB.

As you say, given that airScore has not been released I would change both the DB and the code to allow compatibility with existing DBs. There is no harm in using upper or lower on already correctly formatted strings. But this should be done only when it is for human readable text at point of use not when we read the view otherwise these issues will persist.

biuti commented 3 years ago

I pushed a fix for this (https://github.com/FAI-CIVL/FAI-Airscore/commit/13ef5589e97743fb1507e336c7866937335f3e99). I added a check for lower case, so it should be compatible with older version of view. I also added JTG to elapsed time tasks that was missing. View is updated in Airscore.sql. I tried some different tasks and it worked well.

kuaka commented 3 years ago

still has issues. airscore_prod | File "/app/airscore/core/formulas/libs/leadcoeff.py", line 63, in tot_lc_calc airscore_prod | if res.real_start_time > t.min_dept_time: airscore_prod | TypeError: '>' not supported between instances of 'int' and 'NoneType'

the task in question is elapsed time, which may have not been tested since there were big changes.

any testing of scoring needs to consider race to goal, elapsed time, multi gate, difficulty calc, lead calcs no pilots in goal, all pilots in goal, some pilots in goal, some<min distance etc etc (and all combinations of these.

reloading the tracks and trying to score. I get the same error.

biuti commented 3 years ago

How can min_dept_time be null if you are in lead_coeff_calc? Which task is it?

kuaka commented 3 years ago

I pushed a fix for this (13ef558). I added a check for lower case, so it should be compatible with older version of view. I also added JTG to elapsed time tasks that was missing. View is updated in Airscore.sql. I tried some different tasks and it worked well.

if I read a task from the database that has 'elapsed time' in the database then task_type is still ELAPSED TIME in your commit I cannot see any changes to task.read() which I thought you were going add code to convert to lowercase so that it is compatible with old versions of the view.

biuti commented 3 years ago

I added lower() to all task_type calls in code, that should be enough I think.

kuaka commented 3 years ago

why not do it at read time and be done with it? If you set an elapsed time task, you will see that the task form displays race as it cannot set it to ELAPSED TIME. I have debugged it and the value is ELAPSED TIME and not elapsed time.

I don't understand why we would convert it to lower everywhere and not just when we read it.

biuti commented 3 years ago

if we always use type from task object and not from result files directly, as probably we do, we can convert just reading. I will check.

kuaka commented 3 years ago

as a test I have forced my task to be read 'elapsed time'. I re-uploaded the tracks and it scored without error.

biuti commented 3 years ago

Great, so I'll check if we ever use task_type from json and safely just store lowercase in object, otherwise I'll fix the dropdown in the frontend.

kuaka commented 3 years ago

if we read lower case, why would the json be different? (or are you just allowing for existing files?) The drop down is not broken. It is the data going to the drop down that is broken. If you just 'fix' the drop down the scoring error will persist as the scoring is not using the form but the object.

biuti commented 3 years ago

Yes just for compatibility with old results files

biuti commented 3 years ago

moved to attribute setting with https://github.com/FAI-CIVL/FAI-Airscore/commit/af1cc2bfa84d55ad1fd58e285c88c80d2a2e8c21

kuaka commented 3 years ago

tested and working.