search

yangkevin2 / emnlp22-re3-story-generation

MIT License
244 stars 47 forks source link

Index Out of Range in beam_candidate #3

Closed arnicas closed 1 year ago

arnicas commented 1 year ago

Hi, This work is fab. I think some of the components would even be useful pulled out, frankly. But I've several times gotten this error, i can probably patch locally, but wondering how safe that is. FYI.

File "/home/cherny/code/emnlp22-re3-story-generation/story_generation/draft_module/beam_candidate.py", line 305, in calculate_alignment alignment_score += self.controllers[1]([cut_first_sentence(self.previous_passage(1000)) for _ in range(len(completions))], completions).cpu().numpy() # logprob for alignment with previous story, up to 1k prev tokens IndexError: list index out of range

arnicas commented 1 year ago

oh never mind - it seems i forgot a mandatory controller arg that isn't set by default? And again, now I see the code - maybe this should be controllers[0]. Only one seems to be there by default.

yangkevin2 commented 1 year ago

Hi,

Glad you like the work!

The current code there is correct. It assumes that both a relevance reranker and a coherence reranker are provided in that order (as in the example command in the README), although it's not enforced (and isn't set by default since it wants you to specify a path to the model checkpoints). If you want to patch out that line you'll just end up removing the coherence reranker, which might make the performance a bit worse but shouldn't be make or break.

I should definitely add a check earlier on with a clearer indication of the error though. Will do that later today.

Thanks, Kevin

arnicas commented 1 year ago

Oh i see, I was copying from the notebook code and missed that the same argument was in there twice. Very sorry. Anyway, it's super interesting!