bdilday / pychadwick

Python package to interface with chadwick library
GNU General Public License v2.0
9 stars 4 forks source link

fixes sub event processing bug 🎉 #23

Closed bdilday closed 4 years ago

bdilday commented 4 years ago

There was a bug where around a sub event the gameiter was advanced twice, causing the event following the sub to be absent from the processed events. The relevance of the sub is that prior to a sub, the event is marked NP. The fundamental cause for the following game event to be skipped was that in the orginial c code, the processing happens in the same function as the filtering of NP, where in this modified code the processing has been broken out into its own function (cwevent_process_game_record). In the c code if the event is an NP then the pointer is advanced and the loop is continued without processing the NP event. On the other hand, by putting the processing logic in its own function, it lead to ambiguity about where the filtering of NP happens. I originally had it happen in the cwevent_process_game_record function, so that that function could be self-contained. Unfortunately the filtering also happened in the loop around the calls to cwevent_process_game_record. This PR fixes that faulty logic, such that the filtering of NP only happens in the loop. This better matches the original c code. The downside is the loop has to check the event_text field and avoid passing it to cwevent_process_game_record.

This fix has been tested with the c_chadwick_regression_test.py script using the 1982OAK.EVA file. This probably closes https://github.com/bdilday/pychadwick/issues/9 but I want to double check on additonal events files to be sure.

bdilday commented 4 years ago

I checked this an a large sample of retrosheet event files and found 100% agreement with the c version of chadwick 🎉