Fix type hints for dropout, dropout parameter references, and add docs for FCLayer and FCStack.

[justinxzhao]

No description provided.

[github-actions]

Unit Test Results

       6 files  ±0         6 suites  ±0   2h 24m 31s ⏱️ -41s
2 802 tests +2  2 770 ✔️ +2    32 💤 ±0  0 ❌ ±0 
8 406 runs  +6  8 306 ✔️ +6  100 💤 ±0  0 ❌ ±0 

Results for commit fc7a9c0. ± Comparison against base commit b920c86.

♻️ This comment has been updated with latest results.

[w4nderlust]

It's not super clear to me why in some cases we are renaming parameters from dropout to recurrent dropout and in some cases we are dropping them altogether (no pun intended). Can you give a bit more context @justinxzhao ?

[justinxzhao]

It's not super clear to me why in some cases we are renaming parameters from dropout to recurrent dropout and in some cases we are dropping them altogether (no pun intended). Can you give a bit more context @justinxzhao ?

The changes in this PR is mainly to make the use of separate dropout parameters, where there are already multiple dropout parameters in the constructor, consistent across all Ludwig modules. New consistency in this PR:

• H3RNN:
  • dropout -> H3Embed
  • recurrent_dropout -> RecurrentStack
• StackedRNN:
  • dropout -> EmbedSequence
  • recurrent_dropout -> RecurrentStack
  • fc_dropout -> FCStack
• StackedCNNRNN:
  • dropout -> EmbedSequence
  • conv_dropout -> Conv1DStack
  • recurrent_dropout -> RecurrentStack
  • fc_dropout -> FCStack

As for removing recurrent_dropout=... -- the constructor of RecurrentStack doesn't actually have a kwarg for recurrent_dropout. It's simply called dropout=....

On a side note, I wonder if it's reasonable to simplify/consolidate all of the different dropout parameters into a more global dropout parameter that we can use for everything. Curious to get people's thoughts @w4nderlust @dantreiman @geoffreyangus @ShreyaR

[w4nderlust]

@justinxzhao got it, make sense.
Although it seems to me that StackedCNNRNN has a couple issues.
it doesn't have type hints, the order where the dropout parameter is placed is wrong (it should be close to the embedding parameter at the beginning), in some cases the default value for dropouts is 0, in others it is 0.0 (not a big issue, but consistency is great), and finally in the recurrent stack we have dropout=dropout, but it should be dropout=recurrent_dropout.

A side note: in all these cases, dropout is the one used in embeddings, while other modules have their own. It could be better to rename it to embedding_dropout or embed_dropout for clarity and consistency potentially. What do you think?

[justinxzhao]

I'll defer to #1924 to add type hints everywhere.

Perhaps once we have schemas checked-in, we can use them to automatically generate docstrings from them and do a grand substitution/update everywhere.

Fixed dropout=recurrent_dropout in the StackedCNNRNN.

A side note: in all these cases, dropout is the one used in embeddings, while other modules have their own. It could be better to rename it to embedding_dropout or embed_dropout for clarity and consistency potentially. What do you think?

I'll leave it as dropout for now, to ensure backwards compatibility.

I'm beginning to lean towards consolidating all of the per-module dropout parameters into a single parameter (filed #2080 to track) unless we see strong evidence that dramatically different dropouts results in significantly performance gains. Perhaps we can continue the discussion on that issue, and make that change later.

[geoffreyangus]

LGTM! Left a comment re: dropout here: #2080