Skip to content

Moved GraphMaskExplainer to torch_geometric.explain #7779

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

Merged
merged 34 commits into from
Aug 25, 2023
Merged

Moved GraphMaskExplainer to torch_geometric.explain #7779

merged 34 commits into from
Aug 25, 2023

Conversation

fork123aniket
Copy link
Contributor

@fork123aniket fork123aniket commented Jul 20, 2023

As discussed in #7271 (reply in thread), have attached all the relevant code files to this PR that actually could aid in the final migration of GraphMaskExplainer from torch_geometric.contrib.explain to torch_geometric.explain.

@fork123aniket fork123aniket requested a review from rusty1s as a code owner July 20, 2023 07:18
@rusty1s
Copy link
Member

rusty1s commented Jul 22, 2023

Can we fix the diff here once again?

@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #7779 (385ab50) into master (d6951e7) will decrease coverage by 0.70%.
The diff coverage is 100.00%.

❗ Current head 385ab50 differs from pull request most recent head 2148f5d. Consider uploading reports for the commit 2148f5d to get more accurate results

@@            Coverage Diff             @@
##           master    #7779      +/-   ##
==========================================
- Coverage   90.12%   89.43%   -0.70%     
==========================================
  Files         458      456       -2     
  Lines       26874    26773     -101     
==========================================
- Hits        24221    23945     -276     
- Misses       2653     2828     +175     
Files Changed Coverage Δ
...geometric/explain/algorithm/graphmask_explainer.py 96.26% <ø> (ø)
torch_geometric/contrib/explain/__init__.py 100.00% <100.00%> (ø)
torch_geometric/explain/algorithm/__init__.py 100.00% <100.00%> (ø)

... and 48 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fork123aniket
Copy link
Contributor Author

I just added .gitattributes as done in #7759. Can you check if the Git Diff works as expected??

@rusty1s
Copy link
Member

rusty1s commented Jul 24, 2023

Mh, not really. It still shows full deletion/addition of the code. Let me see what I can do.

@fork123aniket
Copy link
Contributor Author

Yeah, sure. Please let me know if I missed out on committing to this PR any imperative code file which causes this Git Diff issue.

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

The issue around the line endings should be resolved now.

Just curious, what text editor and version of git are you using? 👀

@rusty1s rusty1s changed the title Added GraphMaskExplainer Moved GraphMaskExplainer to torch_geometric.explain Jul 31, 2023
@akihironitta akihironitta self-assigned this Aug 5, 2023
Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@fork123aniket Thank you for working on this. LGTM!

@fork123aniket
Copy link
Contributor Author

@fork123aniket Thank you for working on this. LGTM!

Thanks, @akihironitta. Really appreciate your help here.

@rusty1s @akihironitta When can we expect to have this PR merged, given the changes associated with this PR have already been approved??

@akihironitta akihironitta enabled auto-merge (squash) August 25, 2023 16:03
@akihironitta akihironitta merged commit 143487b into pyg-team:master Aug 25, 2023
@rusty1s
Copy link
Member

rusty1s commented Aug 27, 2023

Thanks for taking care of merging.

jjpietrak pushed a commit that referenced this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants