-
Notifications
You must be signed in to change notification settings - Fork 111
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
Conversation
There was a problem hiding this 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
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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
Currently, I’m only using a single argument, Additionally, I’ve added a prompt to notify users about the setting of |
There was a problem hiding this 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.
Fix the bug in issue #107. Enable to manually set
freq
for the datasets in whichfreq=None
usingpd.infer_freq
. If such datasets are not included infreq_dict
, itsfreq
will be set as 'H' by default.