Skip to content

Conversation

@John-P
Copy link
Contributor

@John-P John-P commented Oct 13, 2021

Add classes and functions under tiatoolbox.tools.graph to enable construction of graphs in a format which can be used with PyG (PyTorch Geometric).

@John-P John-P added the enhancement New feature or request label Oct 13, 2021
@John-P John-P added this to the Release v1.0.0 milestone Oct 13, 2021
@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #155 (09c108f) into develop (333e20b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 09c108f differs from pull request most recent head b3fda91. Consider uploading reports for the commit b3fda91 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           develop     #155    +/-   ##
=========================================
  Coverage    99.78%   99.79%            
=========================================
  Files           46       47     +1     
  Lines         3780     3909   +129     
  Branches       621      646    +25     
=========================================
+ Hits          3772     3901   +129     
  Misses           2        2            
  Partials         6        6            
Impacted Files Coverage Δ
tiatoolbox/tools/graph.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 333e20b...b3fda91. Read the comment docs.

# All rights reserved.
# ***** END GPL LICENSE BLOCK *****

"""Functions to help with constructing graphs."""
Copy link
Member

Choose a reason for hiding this comment

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

if it is just torch based graphs then please mention it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is to expand this later to generic graph construction that we often use, not restricting to just torch. Ideally we would need to package the Graph as some data form and support .to_torch_geoemtry .to_networkx

Copy link
Member

@shaneahmed shaneahmed left a comment

Choose a reason for hiding this comment

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

Thanks @John-P
It looks fine to me. Please make the suggested changes and we can merge. Please get it reviewed by @vqdang and @wenqi006 as well.

@shaneahmed shaneahmed requested review from vqdang and wenqi006 October 18, 2021 08:31
Copy link
Contributor

@vqdang vqdang left a comment

Choose a reason for hiding this comment

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

Need clarification for functionalities. IMO there are some questionable points about the implementation but that can be dealt with later.

@John-P John-P marked this pull request as ready for review October 18, 2021 12:23
@wenqi006
Copy link
Contributor

If we do want to make the graph contractor into an abstract class and have hybrid clustering as one concrete implementation

I do not think it is necessary. But you can go ahead to make the code neat.

@John-P John-P changed the title NEW: Add Graph Construction Functions In Tools NEW: Add Graph Construction In Tools Nov 18, 2021
@John-P John-P changed the title NEW: Add Graph Construction In Tools NEW: Add SlideGraph Construction Method In Tools Nov 20, 2021
Copy link
Member

@shaneahmed shaneahmed left a comment

Choose a reason for hiding this comment

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

Thanks @John-P

Copy link
Member

@shaneahmed shaneahmed left a comment

Choose a reason for hiding this comment

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

Thanks @John-P

@shaneahmed shaneahmed merged commit 866c603 into develop Nov 23, 2021
@shaneahmed shaneahmed deleted the feature-graph-construction branch November 23, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants