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

[refactoring] Shortening the experiment names #47

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

qetdr
Copy link
Collaborator

@qetdr qetdr commented Apr 1, 2023

🔬 Background
Each experiment within a benchmark is given a name. This name is created based on the model name, the dataset set, and the selected parameters. While this is mostly a unique name, it is also very long. Is there a way to adapt the experiment name to be short and unique? Implement an improved experiment name.

🔮 Key changes
The main change was abbreviating the object names in the experiment name.

  1. Reduce the name of a df to max 3 characters.
  2. Reduce the name of a model to use 2-character syllables (e.g., NeuralProphet[Model] -> NePrM)
  3. Create a dictionary (needs to be updated with model parameters) with parameters and their abbreviations. Then, loop over each param (from the model), and abbreviate.
  4. Object separation in the name: '-' separates objects, '_' separates params-args (e.g., parameter:argument -> PM_arg).
  5. Concatenate all to a single string.

Example of the change:

'datasetname_NeuralProphet_seasonality_mode_multiplicative_learning_rate_0.1__data_params__seasonality_mode_ additive, freq_ MS_'

to

'dat-NePrM-SM_mult-LR_0.1-_data_params_-SM_add-freq_MS'

📋 Review Checklist

  • I have performed a self-review of my own code.
  • [NA] I have added docstring
  • [NA] I have added typing
  • I have modified pytests to check against ValueErrors

@qetdr qetdr changed the title Fix exp names [refactoring] Fix exp names Apr 2, 2023
@qetdr qetdr changed the title [refactoring] Fix exp names [refactoring] Shortening the experiment names Apr 2, 2023
@LeonieFreisinger LeonieFreisinger self-requested a review April 2, 2023 17:04
@LeonieFreisinger LeonieFreisinger linked an issue Apr 2, 2023 that may be closed by this pull request
Copy link
Collaborator

@LeonieFreisinger LeonieFreisinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qetdr great work! I very much appreciate the thoughts you put into renaming and like the idea of abbreviating all names. Also, thanks for the detailed description of the PR changes, that makes it easy to understand your code.
The first part of your changes looks good to me. Have another look at the part, where you rename the model parameters. Here we need to have a solution that generalizes well to all possible model parameters.

As some inspiration: Another idea for naming the experiments would be with a numeric unique identifier. However, in this case, we would still need to hold the option for the user, to view or query the underlying configuration. What are your thoughts on this?

Lastly, please check the following:

  • black and flake8 is still failing in your workflows
  • regarding your example 'dat-NePrM-SM_mult-LR_0.1-_data_params_-SM_add-freq_MS' , where does the _ after _data_param come from?

Thank you!!

tot/models/__init__.py Outdated Show resolved Hide resolved
tot/experiment.py Outdated Show resolved Hide resolved
tot/experiment.py Outdated Show resolved Hide resolved
tot/experiment.py Outdated Show resolved Hide resolved
tot/experiment.py Show resolved Hide resolved
tot/experiment.py Outdated Show resolved Hide resolved
abbreviated_name += "M" # for 'Model'

# 3. params (dictionary with abbreviations)
d = {'seasonality_mode': 'SM',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model parameter names can largely vary among the available tsf model. Hence, it might not be a good idea of hard-coding them in a dict. Can you think of another solution?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should find a solution, that generalizes well to all possible model parameters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Experiment name too long
2 participants