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

Major changes #33

Merged
merged 4 commits into from
Oct 19, 2021
Merged

Major changes #33

merged 4 commits into from
Oct 19, 2021

Conversation

RasmusOrsoe
Copy link
Collaborator

This contains main functionality additions.

  • Trainer class (v1.0)
  • Predictor class (v0.1)
  • loss functions
  • dynedge model
  • custom learning rate schedule
  • adhoc utility
  • minor changes to sqlite_dataset.py

All of which are tested (and it works!) in examples/

We now have train/test functionality :-)

@asogaard
Copy link
Collaborator

Hi @RasmusOrsoe,
Thanks for this nice pull request! Brilliant to have the full chain, from data ingestion to model training in the common repo! 🥳
As I mentioned at the weekly meeting, I am happy to review and will focus on the more straightforward stuff (naming conventions, question for clarification, etc.) rather than overall code structure. Once the PR is merged I will try to prepare suggestions for more modular network structure, if it seems promising.

asogaard
asogaard requested changes Oct 19, 2021
View reviewed changes
Copy link
Collaborator

@asogaard asogaard left a comment

Choose a reason for hiding this comment

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

Hi @RasmusOrsoe,
Please see my review comments. Once you have had a change to go over them, I will approve the merge. 👍

src/gnn_reco/components/loss_functions.py Outdated Show resolved Hide resolved
src/gnn_reco/components/loss_functions.py Outdated Show resolved Hide resolved
src/gnn_reco/components/loss_functions.py Outdated Show resolved Hide resolved
src/gnn_reco/components/utils.py Show resolved Hide resolved
src/gnn_reco/components/utils.py Outdated Show resolved Hide resolved
src/gnn_reco/models/dynedge.py Outdated Show resolved Hide resolved
src/gnn_reco/models/dynedge.py Outdated Show resolved Hide resolved
src/gnn_reco/models/convnet.py Show resolved Hide resolved
src/gnn_reco/models/dynedge.py Show resolved Hide resolved
src/gnn_reco/models/utils.py Show resolved Hide resolved
@RasmusOrsoe
Copy link
Collaborator Author

I have addressed the comments from Andreas.

@RasmusOrsoe RasmusOrsoe merged commit 395894a into graphnet-team:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asogaard asogaard asogaard approved these changes

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
@RasmusOrsoe @asogaard