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

Edge properties for reasonable edge classification 📊 #285

Merged
merged 49 commits into from
Oct 13, 2023

Conversation

KristinaUlicna
Copy link
Collaborator

@KristinaUlicna KristinaUlicna commented Oct 3, 2023

PR contribution summary

Why is this PR useful / good for? Please describe the problem(s) you're trying to address.

  • Computes, stores & organises a set of geometrical EdgeProperties
  • Implements edge properties into the classifier training
  • Extends available model catalogue by those which input edge properties
  • Implements the architecture for Graph Attention Network (GAT)
  • Allows [multi-head] attention visualisation to monitor training progress
  • Adds additional config hyperparameters to keep up with the progress

List of proposed changes / linked issues & discussions

What should a reviewer concentrate their feedback on?

  • 🏃 Scripts to run - reproducing training results
  • 💻 Code quality

What type of PR is this? (check all applicable)

  • 🪄 Feature
  • #️⃣ Documentation / code annotation
  • 🧑‍💻 Code refactor / style
  • 🔥 Performance Improvements

Added tests?

  • 🙅 no, temporarily silenced some due to lack of graph attributes

PR review summary

Describe what this PR does & how you reviewed the individual items, where needed:

Some helper checks to tick off:

  • Focus on image annotation
  • Focus on model training
  • Could any optimization be applied?
  • Is there any redundant code?
  • Are there any spelling errors?

In conclusion, after my review, I'd like to:

  • 🙋 ask some clarifying questions
  • 🙅 suggest some specific changes

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@KristinaUlicna KristinaUlicna changed the base branch from main to development October 3, 2023 09:47
@KristinaUlicna KristinaUlicna self-assigned this Oct 3, 2023
@KristinaUlicna KristinaUlicna added tests Automated testing for stable main branch methodology Building functional & diverse pipeline labels Oct 3, 2023
@KristinaUlicna KristinaUlicna marked this pull request as draft October 3, 2023 09:48
@KristinaUlicna
Copy link
Collaborator Author

Temporary silencing some tests which keep failing due to lack of edge properties attribute in the sample graphs. Fix upcoming in subsequent PR(s) 🚀

@KristinaUlicna KristinaUlicna removed the tests Automated testing for stable main branch label Oct 11, 2023
@KristinaUlicna KristinaUlicna marked this pull request as ready for review October 11, 2023 19:15
@KristinaUlicna KristinaUlicna added the visualisation Visualise the data I/O, training progress or properties label Oct 12, 2023
@KristinaUlicna KristinaUlicna added this to the Merge `dev` -> `main` milestone Oct 12, 2023
@crangelsmith
Copy link
Collaborator

I think you can skip tests without having to comment them out with some of these options https://docs.pytest.org/en/7.3.x/how-to/skipping.html

@KristinaUlicna
Copy link
Collaborator Author

Thanks for the suggestion - this is gold 🥇 ! All failing tests are now re-implemented in PR #308 (which extends the failing run if no edge properties are created)

@crangelsmith
Copy link
Collaborator

crangelsmith commented Oct 13, 2023

Ran the training (after calculating node and edge properties beforehand), I get the following results:

MRC_Synthetic_File_006-Whole_Graph_Visualisation
MRC_Synthetic_File_006-Optimised_Components

I think this is compatible with @KristinaUlicna results :)

@crangelsmith
Copy link
Collaborator

My main suggestions (mostly discussed offline with @KristinaUlicna) are being implemented in the following PRs, so happy to approve this one.

@KristinaUlicna KristinaUlicna merged commit 6d5f6c7 into development Oct 13, 2023
1 check passed
@KristinaUlicna KristinaUlicna deleted the edge-properties branch October 13, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
methodology Building functional & diverse pipeline visualisation Visualise the data I/O, training progress or properties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants