search

CUNY-CL / yoyodyne

Small-vocabulary sequence-to-sequence generation with optional feature conditioning
Apache License 2.0
29 stars 16 forks source link

Transducer over-copying #207

Closed Adamits closed 2 months ago

Adamits commented 3 months ago

In #140 I observed the transducer val loss converging but the accuracy staying the same for the english-medium dataset in the sigmorphn-2017 shared task dataset. When predicting, it seemed to learn to primarily copy the lemma.

We should test this on some other datasets, and try to reproduce results with proper hyperparameters, etc, to determine if there is a bug.

In the master branch, predict.py is broken for transducer so we may be blocked by that (#197) for debugging this.

kylebgorman commented 3 months ago

I just trained on Polish abstractness data (--gradient_clip_val 3 --arch transducer --features_encoder linear --max_epochs 50), using CPU, and got absymal performance: validation accuracy < 5%. I believe this was working fine at some earlier point. I didn't bother to predict.

Adamits commented 3 months ago

I just trained on Polish abstractness data (--gradient_clip_val 3 --arch transducer --features_encoder linear --max_epochs 50), using CPU, and got absymal performance: validation accuracy < 5%. I believe this was working fine at some earlier point. I didn't bother to predict.

Does that dataset rely on features?

kylebgorman commented 3 months ago

On Sat, Jul 6, 2024 at 8:07 PM Adam @.***> wrote:

I just trained on Polish abstractness data (--gradient_clip_val 3 --arch transducer --features_encoder linear --max_epochs 50), using CPU, and got absymal performance: validation accuracy < 5%. I believe this was working fine at some earlier point. I didn't bother to predict.

Does that dataset rely on features?

Yes, I used the features column. The task is harder without them.

K

— Reply to this email directly, view it on GitHub https://github.com/CUNY-CL/yoyodyne/issues/207#issuecomment-2212057393, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG4ONBH6XUDOHFPM3RIWDZLCBFFAVCNFSM6AAAAABKOWETZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJSGA2TOMZZGM . You are receiving this because you commented.Message ID: @.***>

Adamits commented 3 months ago

Ok thanks. I think I did introduce a bug in the PR we just merged.

When I compute the source_vocab_size here: https://github.com/CUNY-CL/yoyodyne/blob/master/yoyodyne/models/transducer.py#L42 I believe this assumes we do not have tied embeddings. If embeddings are tied, then source_vocab_size == vocab_size iirc. This means with tied_embeddings (the default), the transducer index is messed up. I think this could explain converging val_loss but not accuracy---a bunch of tokens are probably erroneously shifted, and val accuracy needs to access the actually decoded strings. It may also explain the copying if the target copy action is intact, but insertions are incorrect?

To solve this I think the transducer needs to somehow know if we have tied embeddings or not. I need to think on it more to be sure though.

kylebgorman commented 3 months ago

When I compute the source_vocab_size here: https://github.com/CUNY-CL/yoyodyne/blob/master/yoyodyne/models/transducer.py#L42 I believe this assumes we do not have tied embeddings. If embeddings are tied, then source_vocab_size == vocab_size iirc. This means with tied_embeddings (the default), the transducer index is messed up. I think this could explain converging val_loss but not accuracy---a bunch of tokens are probably erroneously shifted, and val accuracy needs to access the actually decoded strings. It may also explain the copying if the target copy action is intact, but insertions are incorrect?

Makes sense so far.

To solve this I think the transducer needs to somehow know if we have tied embeddings or not. I need to think on it more to be sure though.

Well here's the good news: you yourself added code in your last PR that ensured that the transducer can only be used with tied embeddings ;)

Adamits commented 2 months ago

Well here's the good news: you yourself added code in your last PR that ensured that the transducer can only be used with tied embeddings ;)

Ah I had somehow forgotten---good memory! That is good news. The bad news is that this does not seem to fix the issue.

I have been trying different commits (across branches that will not have #140 in the commit history) and so far am pretty confused about the source of the bug. Will report back when I find something useful.

Adamits commented 2 months ago

I tested a ton of different commits and could not target a bug that way. I decided to go back to working on the master branch, and noticed a small eval issue that seemed unrelated. I fixed this issue in #228 and tested the transducer on English medium from the SIGMORPHON 2017 inflection data. This seems to be working as expected now. We can close this once #228 is mergedl