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

[BugFix] Return a list of ntypes/etypes #2742

Merged
merged 3 commits into from
May 13, 2021
Merged

Conversation

chwan1016
Copy link
Contributor

@chwan1016 chwan1016 commented Mar 12, 2021

Description

The comment of function load_partition shows that two return variables ntype and etype are lists. After running the code, I notice that the actual types are dictionaries. I guess returning lists is preferred, otherwise the comment should be fixed.

Note that there is no test that is related to this change.

Checklist

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [$CATEGORY] (such as [NN], [Model], [Doc], [Feature]])
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented
  • To the my best knowledge, examples are either not affected by this change,
    or have been fixed to be compatible with this change
  • Related issue is referred in this PR

Changes

@chwan1016 chwan1016 changed the title [Feature] return a list of ntypes/etypes [BugFix] Return a list of ntypes/etypes Mar 22, 2021
@zheng-da
Copy link
Collaborator

It seems there is some inconsistency between the comments and the code. Thanks for fixing the problem.
By checking how load_partition and load_partition_book are used, I think it's better to make the returned results by these two functions consistent. Please check out how these two functions are used here.
I think our option is either returning dicts in both functions and changing the docstring of load_partition, or returning lists and changing the docstring of load_partition_book.

@zheng-da zheng-da merged commit e156331 into dmlc:master May 13, 2021
@chwan1016 chwan1016 deleted the patch-1 branch November 14, 2022 06:32
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.

2 participants