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

Manually set 'freq' for specific datasets #110

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

zqiao11
Copy link
Contributor

@zqiao11 zqiao11 commented Aug 19, 2024

Fix the bug in issue #107. Enable to manually set freq for the datasets in which freq=None using pd.infer_freq. If such datasets are not included in freq_dict, its freq will be set as 'H' by default.

@liu-jc liu-jc self-requested a review August 19, 2024 13:02
Copy link
Contributor

@liu-jc liu-jc left a comment

Choose a reason for hiding this comment

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

I think we may want to avoid freq_dict and add an additional feature. See the detailed comments. Feel free to share your thoughts on this suggestion.

src/uni2ts/data/builder/simple.py Outdated Show resolved Hide resolved
from uni2ts.data.dataset import EvalDataset, SampleTimeSeriesType, TimeSeriesDataset
from uni2ts.data.indexer import HuggingFaceDatasetIndexer
from uni2ts.transform import Transformation

from ._base import DatasetBuilder
# Manually set the freq of the datasets whose freq can be inferred automatically. Default freq is H.
freq_dict = defaultdict(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking maybe we can have an argument as in the main function like 'set_freq' and another argument for user-defined freq. Something like the following:

    # Define the `freq` argument with a default value
    parser.add_argument(
        '--freq',
        default='H',  # Set the default value
        help="The user specified frequency"
    )

    # Define an additional flag to detect if `--freq` is explicitly set
    parser.add_argument(
        '--set_freq',
        action='store_true',
        help="Flag to check if `freq` was explicitly set"
    )

Then if the command has set_freq, we directly set the freq to the user specified frequency. Otherwise, we use the current logic (i.e., infer the freq and if it's None, set to 'H'). Does it make sense?

I think in this way, we can avoid the hard-coded freq_dict and add more flexibilities for user to set the frequency. This additional feature should not affect the current command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this method is better and more user-friendly. I will update accordingly. Thanks for the suggestions!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can only use one argument? If freq is not given/modified in the command, the codes will use 'H' by default. It seems not necessary to use another set_freq argument.

@zqiao11
Copy link
Contributor Author

zqiao11 commented Aug 20, 2024

Currently, I’m only using a single argument, freq. From my perspective, if freq can be inferred automatically, there's no need to enable users to set it manually to another predefined value.

Additionally, I’ve added a prompt to notify users about the setting of freq. This lets them know if the freq of a dataset can be inferred automatically. If not, they can manually specify the correct freq with the argument. Without such prompting, it would be difficult for the users to notice if a mismatched freq is used.

@liu-jc liu-jc self-assigned this Aug 22, 2024
Copy link
Contributor

@liu-jc liu-jc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your help. I will merge this.

@liu-jc liu-jc merged commit 901a987 into SalesforceAIResearch:main Aug 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants