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

general split_dataset functions #100

Merged
merged 2 commits into from
Nov 16, 2023
Merged

general split_dataset functions #100

merged 2 commits into from
Nov 16, 2023

Conversation

EnricoTrizio
Copy link
Collaborator

@EnricoTrizio EnricoTrizio commented Nov 15, 2023

Description

Create a single split_dataset function to split datasets in sequential and random mode to fix #91
This deprecates the sequential_split function, which becomes a case of the more general split_dataset function.

Questions

  • By default, DeprecationWarnings are not shown. Is it better to print a UserWarning for the sequential_split function?

Status

  • Ready to go

@EnricoTrizio EnricoTrizio added the bug Something isn't working label Nov 15, 2023
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #100 (c9f6138) into main (6e378a6) will decrease coverage by 0.03%.
The diff coverage is 73.33%.

Additional details and impacted files

Copy link
Collaborator

@andrrizzi andrrizzi left a comment

Choose a reason for hiding this comment

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

Looks good! The only thing I'd change is to pass stacklevel=2 to warnings.warn() to print the location of the code that calls the function rather than the location where the function is defined.

By default, DeprecationWarnings are not shown. Is it better to print a UserWarning for the sequential_split function?

Yes, I think DeprecationWarning is ignored by default. There are two solutions. I'd recommend using FutureWarning instead, which is supposed to be still to flag deprecation but to users rather than developers, and it should be printed by default (do test this however). Otherwise, temporarily set the filter might work as well. Something like

warnings.simplefilter('always', category=DeprecationWarning)
warnings.warn(...)
warnings.simplefilter('default', category=DeprecationWarning)

@EnricoTrizio EnricoTrizio merged commit d14cf54 into main Nov 16, 2023
12 checks passed
@EnricoTrizio EnricoTrizio deleted the fix_split_function branch November 16, 2023 10:44
@EnricoTrizio
Copy link
Collaborator Author

Happy 100th PR guys 🥇 🎉
This fixes #91

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

Successfully merging this pull request may close these issues.

Bug in building DictModule with old version of PyTorch.
2 participants