Skip to content

Master forpr#6

Open
Yuang-Deng wants to merge 51 commits intomasterfrom
master_forpr
Open

Master forpr#6
Yuang-Deng wants to merge 51 commits intomasterfrom
master_forpr

Conversation

@Yuang-Deng
Copy link
Copy Markdown
Owner

Integrate autogluon to tsbench

Copy link
Copy Markdown
Owner Author

@Yuang-Deng Yuang-Deng left a comment

Choose a reason for hiding this comment

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

clean the code

@Yuang-Deng Yuang-Deng marked this pull request as ready for review July 6, 2022 02:36
Copy link
Copy Markdown

@jaheba jaheba left a comment

Choose a reason for hiding this comment

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

Can you ensure to run black for unified code formatting?

Also, I'm wondering why lines are commented out instead of removing them?

fastparquet = "^0.6.1"
fbprophet = "^0.7.1"
gluonts = {git = "https://github.com/awslabs/gluon-ts.git", rev = "7c94c1149875f6ad2e0d7b0a6bcee952f14d3fb1"}
gluonts = "0.9.5"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we use 0.10?

I also would not fix the patch version.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok, I will change it.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I notice that version 0.10 has removed save_datasets, save_metadata, is there an alternate interface?


from ._main import datasets
from .compute_catch22 import compute_catch22 # type: ignore
# from .compute_catch22 import compute_catch22 # type: ignore
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not just remove this?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This function rely on a package called catch22, but this package was renamed suddenly, and there was a problem with the installation, and the old version of the package could not be installed, so we comment it and will add it after solving the problem of this package.

for config in tqdm(DATASET_REGISTRY.values()):
upload_directory(
s3, path / config.name(), bucket, f"{prefix}/{config.name()}"
s3, path + '/' + config.name(), bucket, f"{prefix}/{config.name()}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Since both path and config.name() are of type str, "path / config.name()" will raise an error.

The evaluations are downloaded to the provided directory.
"""
target = Path(evaluations_path)
target = Path.joinpath(target, experiment)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
target = Path.joinpath(target, experiment)
target /= experiment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

target = Path(evaluations_path) / experiment
we might be able to do this with one line of code?

Comment on lines +23 to +27
try:
# TODO this problem may be caused by the version of gluonts?
from gluonts.support.util import maybe_len
except:
from gluonts.itertools import maybe_len
Copy link
Copy Markdown

@jaheba jaheba Jul 7, 2022

Choose a reason for hiding this comment

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

If the gluon-ts version is fixed, only one should be necessary.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok, I will fix the version of gluon-ts and remove redundancy code

all_configurations = generate_configurations(Path(config_path))

# Then, we can run the training, passing parameters as required
source_bucket_prefix = "source"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In this case, I would just add the "source" to the code_location path since we are not expect anyone to change it.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I will add a parameter to control the code_location path


METRICS = ["mase", "smape", "nrmse", "nd", "ncrps"]

DATASETS = ["m3_yearly", "m3_quarterly", "m3_monthly", "m3_other", "m4_quarterly", "m4_monthly",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could we add a "datasets" options so we don't have to modify the file every rime?

res['model'] = model
res['dataset'] = ds
res.update(performance['performances'][-1]['testing'])
val_loss = performance['performances'][n-1]['evaluation']['val_loss'] if 'evaluation' in performance['performances'][n-1] else -1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you add a comment on why this is needed?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also, here we assume we always take the last model. Actually when there are multiple epochs, we may want to take the iteration with the minimum val_loss.

assert name in DATASET_REGISTRY, f"Dataset name '{name}' is unknown."
return DATASET_REGISTRY[name](Path(path))

def construct_pandas_frame_from_iterable_dataset(
Copy link
Copy Markdown

@huibinshen huibinshen Jul 11, 2022

Choose a reason for hiding this comment

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

Is this already supported in autogluon.timeseries? And if this is not used, we could remove it.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

ok, this code was created when I integrated auto pytorch, it is for testing, I will remove it

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.

3 participants