Update field descriptions for ludwig-docs #3123

tgaddair · 2023-02-20T21:37:52Z

No description provided.

for more information, see https://pre-commit.ci

connor-mccorm

This is an awesome enhancement. Not only does this make things more standardized for the repeated parameters, but the new info you've added is very insightful and helpful. I just added in a few thoughts I had while reading through the new metadata but overall love it. Thank you!

connor-mccorm · 2023-02-20T22:35:20Z

ludwig/schema/metadata/configs/common.yaml

+    on the computational load of the model and might require further hypterparameter
+    tuning
+  expected_impact: 2
+  suggested_values: The default value will work well in the majority of the


Thinking maybe this field should be empty or None and then we put this text under suggested_value_reasoning. WDYT?

Good call, done.

connor-mccorm · 2023-02-20T22:46:29Z

ludwig/schema/common_fields.py

+def DropoutField(default: float = 0.0, description: str = None, parameter_metadata: ParameterMetadata = None) -> Field:
+    description = description or (
+        "Default dropout rate applied to fully connected layers. "
+        "Increasing dropout is a common form of regularization to combat overfitting."


Could be useful to add in here or in common.yaml what exactly the percentage value is doing - i.e. 0.85 means that 85% of the nodes in that layer will be kept or 85% will be dropped - or something along those lines. Reason I say that is that personally, when I've looked into dropout in the past, it's a fairly easy idea to conceptualize but I've experienced different tools interpreting the input value in different ways. For instance, I've seen 0.85 mean drop 85% in one tool and keep 85% in another. So I feel like specifying this somewhere - probably common.yaml in suggested_values_reasoning as I'm writing this out - could be valuable, since the user will likely look this up and have a similar question. WDYT?

Very good callout. Will put this in the description.

connor-mccorm · 2023-02-20T22:53:35Z

ludwig/schema/combiners/tabnet.py

@@ -22,7 +22,9 @@ def module_name():
    )

    size: int = schema_utils.PositiveInteger(
-        default=32, description="`N_a` in the paper.", parameter_metadata=COMBINER_METADATA["TabNetCombiner"]["size"]
+        default=32,
+        description="Size of the hidden layers. `N_a` in the paper.",


No AI here, just a thought: I don't think our descriptions render text surrounded in backticks as a code snippet yet. This would be an improvement that makes the UI a little crisper and more meaningful.

Good point, it's mostly useful for the ludwig docs at the moment, which uses markdown.

connor-mccorm · 2023-02-20T23:01:37Z

ludwig/schema/metadata/configs/common.yaml

+    - https://machinelearningmastery.com/batch-normalization-for-training-of-deep-neural-networks/
+  related_parameters:
+    - norm_params
+  suggested_values: '"batch" or "layer"'


Don't know if we should remove these necessarily, but I've always thought it a little unhelpful that the suggested values here are just both of the available values. I get that they are better than None, but I feel like from 85% of user's view they will see this and think that it's not very helpful since it's essentially suggesting that if the user is going to use this parameter they should use it. There are a couple other instances similar to this one throughout the metadata so do you think it would make sense to remove the suggested value here or just leave it in and gloss over it?

Agreed, will remove.

github-actions · 2023-02-20T23:09:56Z

Unit Test Results

        6 files ±  0         6 suites ±0 5h 46m 7s ⏱️ - 14m 42s
  3 961 tests +  5   3 924 ✔️ +  5   37 💤 ±0 0 ❌ ±0
11 839 runs +15 11 729 ✔️ +15 110 💤 ±0 0 ❌ ±0

Results for commit d5e2bac. ± Comparison against base commit c638c23.

♻️ This comment has been updated with latest results.

…into up-descriptions

for more information, see https://pre-commit.ci

tgaddair · 2023-02-21T00:17:20Z

Thanks for the great comments, @connor-mccorm!

…into up-descriptions

ksbrar

👌🏽

ksbrar · 2023-02-21T05:02:10Z

ludwig/schema/common_fields.py

+    )
+
+
+INITIALIZER_SUFFIX = """


Note that this won't be possible after #3075

Yes, true, we will need to refactor this.

ksbrar · 2023-02-21T05:07:07Z

ludwig/schema/combiners/comparator.py

@@ -12,6 +14,13 @@
 class ComparatorCombinerConfig(BaseCombinerConfig):
    """Parameters for comparator combiner."""

+    def __post_init__(self):


Perhaps better placed in the config_validation.checks suite. Or we should decentralize that suite when you have schema-specific requirements (e.g. see check_sequence_concat_combiner_requirements).

I originally had it there, but it was very ugly and hacky. I actually much prefer having it here, to better encapsulate the validation logic. In the future, we can look to remove this by combining num_fc_layers and fc_layers into a single oneOf option.

ksbrar · 2023-02-21T05:09:30Z

ludwig/schema/combiners/tabnet.py

@@ -22,7 +22,9 @@ def module_name():
    )

    size: int = schema_utils.PositiveInteger(
-        default=32, description="`N_a` in the paper.", parameter_metadata=COMBINER_METADATA["TabNetCombiner"]["size"]
+        default=32,
+        description="Size of the hidden layers. `N_a` in the paper.",


ksbrar · 2023-02-21T05:12:37Z

ludwig/schema/combiners/tabnet.py

        parameter_metadata=COMBINER_METADATA["TabNetCombiner"]["output_size"],
    )

    num_steps: int = schema_utils.NonNegativeInteger(
        default=3,
        description="Number of steps / repetitions of the the attentive transformer and feature transformer "
-        "computations. `N_steps` in the paper ",
+        "computations. `N_steps` in the paper.",


Nit: I know it's a seminal paper but perhaps we should say (Arik and Pfister, 2019) instead of literally the paper.

Good point, I agree that's. good change.

ksbrar · 2023-02-21T05:14:31Z

ludwig/schema/common_fields.py

+        "Requires all fully connected layers to have the same `output_size`."
+    )
+    parameter_metadata = parameter_metadata or COMMON_METADATA["residual"]
+    return schema_utils.Boolean(


idk if this is that much more readable/efficient but since we're passing all local variables you could say e.g. Boolean(**locals())?

That's true, but a little dangerous if someone ever adds a new local var.

ksbrar · 2023-02-21T05:17:11Z

ludwig/schema/optimizers.py

@@ -502,15 +502,25 @@ class GradientClippingConfig(schema_utils.BaseMarshmallowConfig):
    """Dataclass that holds gradient clipping parameters."""


Would you mind removing desciption = TODO from OptimizerDataclassField above? :D

tgaddair and others added 5 commits February 19, 2023 22:09

Added common fields

1d8588c

Common params

aa46eb9

Sequence concat

ad5d9ec

Cleanup

39d6aee

[pre-commit.ci] auto fixes from pre-commit.com hooks

daaefa4

for more information, see https://pre-commit.ci

tgaddair requested review from ksbrar and connor-mccorm February 20, 2023 21:45

connor-mccorm approved these changes Feb 20, 2023

View reviewed changes

tgaddair and others added 6 commits February 20, 2023 15:50

Fix

3f893ae

Checks

b7a86f9

Merge branch 'up-descriptions' of https://github.com/ludwig-ai/ludwig …

8294125

…into up-descriptions

Dropout

cb36ac9

Addressed comments

66ef92b

[pre-commit.ci] auto fixes from pre-commit.com hooks

21e1497

for more information, see https://pre-commit.ci

tgaddair added 2 commits February 20, 2023 19:43

Better check

cb0569e

Merge branch 'up-descriptions' of https://github.com/ludwig-ai/ludwig …

e9fdf30

…into up-descriptions

ksbrar approved these changes Feb 21, 2023

View reviewed changes

Addressed comments

d5e2bac

tgaddair merged commit 0b8765b into master Feb 21, 2023

tgaddair deleted the up-descriptions branch February 21, 2023 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update field descriptions for ludwig-docs #3123

Update field descriptions for ludwig-docs #3123

tgaddair commented Feb 20, 2023

connor-mccorm left a comment

connor-mccorm Feb 20, 2023

tgaddair Feb 21, 2023

connor-mccorm Feb 20, 2023

tgaddair Feb 21, 2023

connor-mccorm Feb 20, 2023

tgaddair Feb 21, 2023

ksbrar Feb 21, 2023

connor-mccorm Feb 20, 2023

tgaddair Feb 21, 2023

github-actions bot commented Feb 20, 2023 •

edited

Loading

tgaddair commented Feb 21, 2023

ksbrar left a comment

ksbrar Feb 21, 2023

tgaddair Feb 21, 2023

ksbrar Feb 21, 2023

tgaddair Feb 21, 2023

ksbrar Feb 21, 2023

ksbrar Feb 21, 2023

tgaddair Feb 21, 2023

ksbrar Feb 21, 2023

tgaddair Feb 21, 2023

ksbrar Feb 21, 2023

		@@ -502,15 +502,25 @@ class GradientClippingConfig(schema_utils.BaseMarshmallowConfig):
		"""Dataclass that holds gradient clipping parameters."""

		)


		INITIALIZER_SUFFIX = """

Update field descriptions for ludwig-docs #3123

Update field descriptions for ludwig-docs #3123

Conversation

tgaddair commented Feb 20, 2023

connor-mccorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2023 • edited Loading

Unit Test Results

tgaddair commented Feb 21, 2023

ksbrar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2023 •

edited

Loading