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

[Feature] Add bias in node decoder #1111

Merged
merged 19 commits into from
Jan 3, 2025
Merged

[Feature] Add bias in node decoder #1111

merged 19 commits into from
Jan 3, 2025
+152 −23

Conversation

RonaldBXu
Copy link
Contributor

@RonaldBXu RonaldBXu commented Dec 11, 2024 •
edited
Loading

Issue #, if available:

#335

Description of changes:

Adding bias in node decoder and unit tests.

Regression Test W/ Bias:

Dataset		Test Performance	Validation Performance	Total Time	Epoch Time/Seconds
OGB-MAG-NC	Acc: 0.4023		Acc: 0.4149		0:24:55		68.511
OGB-PROD-NC	Acc: 0.7178		Acc: 0.9117		0:47:22		110.806

Regression Test W/O Bias:

Dataset		Test Performance	Validation Performance	Total Time	Epoch Time/Seconds
OGB-MAG-NC	Acc: 0.4067		Acc: 0.4113		0:23:56		63.323
OGB-PROD-NC	Acc: 0.7250		Acc: 0.9138		0:46:08		104.001

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@RonaldBXu RonaldBXu self-assigned this Dec 11, 2024
@RonaldBXu RonaldBXu changed the title add dropout and bias in node decoder [Draft] [Feature] Add dropout and bias in node decoder Dec 12, 2024
@RonaldBXu RonaldBXu marked this pull request as ready for review December 28, 2024 22:19
@RonaldBXu RonaldBXu changed the title [Draft] [Feature] Add dropout and bias in node decoder [Feature] Add dropout and bias in node decoder Dec 28, 2024
zhjwy9343
zhjwy9343 reviewed Dec 30, 2024
View reviewed changes
Copy link
Contributor

@zhjwy9343 zhjwy9343 left a comment

Choose a reason for hiding this comment

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

This PR added decoder bias to node decoder only. Any reason why no add to edge decoders?

python/graphstorm/config/argument.py Outdated Show resolved Hide resolved
python/graphstorm/config/argument.py Outdated Show resolved Hide resolved
python/graphstorm/config/argument.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
tests/unit-tests/test_decoder.py Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
classicsong
classicsong reviewed Dec 30, 2024
View reviewed changes
python/graphstorm/config/argument.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
tests/unit-tests/test_decoder.py Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
@RonaldBXu
Copy link
Contributor Author

@zhjwy9343 I think Xiang said to just do node decoder bias for now and edge decoder bias in a later pr.

classicsong
classicsong reviewed Dec 31, 2024
View reviewed changes
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
jalencato
jalencato reviewed Dec 31, 2024
View reviewed changes
Copy link
Collaborator

@jalencato jalencato left a comment

Choose a reason for hiding this comment

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

Could you add some performance number in the PR description for w/wo bias?

python/graphstorm/config/argument.py Outdated Show resolved Hide resolved
python/graphstorm/model/node_decoder.py Outdated Show resolved Hide resolved
@zhjwy9343 zhjwy9343 added ready able to trigger the CI 0.4 labels Jan 2, 2025
@RonaldBXu RonaldBXu changed the title [Feature] Add dropout and bias in node decoder [Feature] Add bias in node decoder Jan 2, 2025
zhjwy9343
zhjwy9343 reviewed Jan 2, 2025
View reviewed changes
tests/unit-tests/test_decoder.py Show resolved Hide resolved
tests/unit-tests/test_decoder.py Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
tests/unit-tests/test_decoder.py Outdated Show resolved Hide resolved
classicsong
classicsong approved these changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@classicsong classicsong left a comment

Choose a reason for hiding this comment

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

LGTM.

zhjwy9343
zhjwy9343 approved these changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@zhjwy9343 zhjwy9343 left a comment

Choose a reason for hiding this comment

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

LGTM now

@RonaldBXu RonaldBXu merged commit e6c2eaa into awslabs:main Jan 3, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhjwy9343 zhjwy9343 zhjwy9343 approved these changes

@classicsong classicsong classicsong approved these changes

@jalencato jalencato Awaiting requested review from jalencato

Assignees

@RonaldBXu RonaldBXu

Labels
0.4 ready able to trigger the CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RonaldBXu @classicsong @zhjwy9343 @jalencato