opened
jacksonjreed
username_0
There's some pretty big changes in here that may be controversial, so I'll try to give a good overview of what I've done.
This is a rewrite of rolling window to avoid redundant calculations that were seriously holding back the tool in terms of speed and memory usage. The main idea is that **instead of instantiating a string for each window and searching for the terms in each of them**, this new version:
* Just iterates through one copy of the passage using an index
* Only analyzes content rolling into and out of the window, ignoring any overlap in the middle.

### Good things:
* This version produces identical results to the old version; I verified this in several different configurations.
* Speed and memory usage are now minimally, if at all, impacted by window size.
* The data is generated as much as 30x faster in some cases; at worst, it takes about the same amount of time. With a bigger window, the speedup is more dramatic. Certain configurations benefit more, e.g. word search in word window.
* I was able to keep most of the structure of the file intact, making major changes to one function and some minimal changes to a few others.

### Less good things:
* Regex searches still search the entirety of every window, so the speed will be about the same.
* "Rolling ratio" mode is still attached to the old functionality; to make it work with this new method would take a more significant structural overhaul. It's still totally functional, but will perform as before.
* The code style is not great; the speedup comes at the cost of generalization and compartmentalization, so several functions ended up consolidated into one big, long function containing a number of helper functions that specifically handle different cases. This results in some messy scoping and lots of line wraps; looking for feedback on this. Perhaps the structure could be improved without sacrificing the functionality.
* This will probably break most of the unit tests for RWA, which I have yet to update.

I'm hoping to get some serious review on this before anything gets merged; @Weiqi97 @username_2 @username_1 thoughts?
Rolling window rewrite
issue
created
scottkleinman
username_1
It looks like this got merged without the requested feedback. Since it seems to perform as well as the old code, this is fine. But it might be a good idea to submit an issue so that the code can undergo further review in the future.
comment
created
chantisnake
username_2
1. There are some obvious violation of the style guide to trade for memory efficiency. 
2. Functions and methods are way too long
3. There are some code that I don't understand after a brief scan, but seems hacky, like adding space after every word. String manipulation are never safe, data structures are much better than string. We should only use string manipulation for io purpose, we should not have any string manipulation in the core logic.
4. Logic are very complicated, we have huge block of if nested in for nested in if statements.

Overall, this code definitely could use some further modulization, which will solve all the problems above.
comment
created
jacksonjreed
username_0
group consensus on this was to leave it merged for now (as nothing is functionally broken and performance seems better) and open an issue to improve style + remove unsafe sections later on--hopefully I can get some work in on this next week.
comment