Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default values for val_steps_to_log and ar_steps_eval don't match #120

Open
sadamov opened this issue Feb 10, 2025 · 3 comments
Open

Default values for val_steps_to_log and ar_steps_eval don't match #120

sadamov opened this issue Feb 10, 2025 · 3 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@sadamov
Copy link
Collaborator

sadamov commented Feb 10, 2025

As the title says, there are not enough ar_steps to log, by default, leading to an error on training start:

    parser.add_argument(
        "--ar_steps_eval",
        type=int,
        default=10,
        help="Number of steps to unroll prediction for during evaluation "
        "(default: 10)",
    )
    parser.add_argument(
        "--val_steps_to_log",
        nargs="+",
        type=int,
        default=[1, 2, 3, 5, 10, 15, 19],
        help="Steps to log val loss for (default: 1 2 3 5 10 15 19)",
    )

I suggest to change the val_steps_to_log to default=[1, 2, 3, 5, 7, 9] and its docstring.

@sadamov sadamov added bug Something isn't working good first issue Good for newcomers labels Feb 10, 2025
@joeloskarsson
Copy link
Collaborator

This is the actual issue causing the error in #105. I suggest to close that and keep track of this here (better documented here).

@leifdenby
Copy link
Member

I suggest to change the val_steps_to_log to default=[1, 2, 3, 5, 7, 9] and its docstring.

Rather than repeating the default variables in the help string I think we should use the ArgumentDefaultsHelpFormatter https://docs.python.org/3/library/argparse.html#argparse.ArgumentDefaultsHelpFormatter in argparse. Also I would make the two defaults be defined from a single list, something like:

DEFAULT_EVAL_STEPS_TO_LOG = [1, 2, 3, 5, 7, 9]
DEFAULT_NUM_EVAL_STEPS = max(DEFAULT_EVAL_STEPS_TO_LOG)

parser = argparse.ArgumentParser(formatter_class=argparse.ArgumentDefaultsHelpFormatter, ...)
parser.add_argument(
        "--val_steps_to_log",
        nargs="+",
        type=int,
        default=[v for vin DEFAULT_VAL_STEPS if v <= DEFAULT_NUM_EVAL_STEPS,
        help="Steps to log val loss for",
    )

@joeloskarsson
Copy link
Collaborator

I've been thinking for ages that we should add ArgumentDefaultsHelpFormatter to all argparse uses, but keep forgetting to write up an issue about it 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants