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.

Sign up for GitHub

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

Jump to bottom

Fixed some bugs (see commit details and implementation for details) #14

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fixed some bugs (see commit details and implementation for details) #14

wants to merge 8 commits into from

Conversation

Syqinx
Copy link

@Syqinx Syqinx commented May 1, 2024

已更正代码中的一些bug，请审查

Syqinx added 3 commits April 30, 2024 11:14
添加未定义函数的实现
b9d7341 
dataloader.py中get_idx_split函数中用到了未定义的函数rand_train_test_idx，本次提交添加了该函数的实现
修改当命令行参数num_exp>1时出现的bug
cac4564 
因为当命令行参数num_exp>1时，调用repeat_run(args)函数，该函数里多次调用run(args)函数，而run(args)函数里当出现某些命令行参数时会去修改输出路径，原实现会导致输出路径重复拼接而导致输出路径错误，从而导致运行出错。本次提交修改了这个bug
修改train_student时因为num_exp>1时的bug
da6f2de 
当传入的命令行参数中num_exp>1且含有特定一些参数时(如feature_noise、feature_noise等)会发生输出路径output_dir重复拼接的错误，本次提交修正了这个bug
@Syqinx
Copy link
Author

Syqinx commented May 1, 2024

@ShichangZh

@ShichangZh
Copy link
Contributor

Thank you so much for bringing up these issues and providing potential solutions to help improve our code quality. I have a few comments.

  1. For the output_path, it is indeed a bug when special parameters like feature_noise are set and num_exp > 1. Thank you for catching it. However, regarding the fix, I would prefer accessing num_exp inside the run function with args.num_exp instead of adding an extra argument to the run function. This will keep the function arguments clean.
  2. For the implementation of the rand_train_test_idx function. I think there could be a mismatch between the intended non-negative node index and the actual non-negative node index when ignore_negative is set to true, as label = label[label >= 0] changes all the node indices without mapping them back.
  3. What are these .idea/ files for?

Please kindly let me know whether you would agree with these comments.

Fixed a bug from the last submission
164a79e 
Use args.seed instead of additional parameters as the current state of the experiment;
Fixed a bug in the rand_train_test_idx function in dataload.py;
From .gitignore exclude the folder .idea/
@Syqinx
Copy link
Author

Syqinx commented May 2, 2024

  1. Yes! You are right, but I think the parameter args.seed is a better choise, Because args.num_exp does not represent the current state of the experiment, it is just a number that specifies the number of times the experiment is performed.
  2. Yes! Thank you for your correction!
  3. .idea/ folder was added automatically when I imported the project into IDE-PyCharm, and I forgot to exclude it from the .gitignore file.

@Syqinx
Copy link
Author

Syqinx commented May 2, 2024

@ShichangZh

@ShichangZh
Copy link
Contributor

Thank you for the quick update. I agree that using args.seed is a more reasonable choice for the first point. For the second point of the rand_train_test_idx function, a minor comment is that the variable name valid_idx is overloaded for "valid (nonnegative) labels" and "labels for validation". Maybe the first valid_idx in line 253 can be renamed to something like nonnegative_idx to avoid confusion?

@ShichangZh
Copy link
Contributor

The fix looks good to me. I think this pull request can be merged. @nshah171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Syqinx @ShichangZh