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

Directed line graph #178

Merged
merged 15 commits into from
Oct 3, 2023
Merged

Directed line graph #178

merged 15 commits into from
Oct 3, 2023

Conversation

lbluque
Copy link
Contributor

@lbluque lbluque commented Oct 2, 2023

Summary

Implements constructing a directed line graph, where edges in the original graph have an edge in the line graph if the dst node of the first edge matches the src node of the second. i.e. the following directed edges x = (u,v) and y = (v,w) will have a directed edge in the line graph (x, y).

Implemented:

  • create_directed_line_graph
  • ensure_directed_line_graph_compatibility to be used when a graph and line graph are passed (ie when training)
  • prune_edges_by_features to prune graphs according to some feature.

I had to adapt compute_theta to handle the "directed" case, bc in this case one of the bond vectors must be flipped to get the correct angle and not its complement.

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (13a5da2) 97.97% compared to head (e32faa7) 97.99%.
Report is 1 commits behind head on main.

❗ Current head e32faa7 differs from pull request most recent head 52d1e17. Consider uploading reports for the commit 52d1e17 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   97.97%   97.99%   +0.01%     
==========================================
  Files          28       28              
  Lines        1828     1891      +63     
==========================================
+ Hits         1791     1853      +62     
- Misses         37       38       +1     
Files Coverage Δ
matgl/graph/compute.py 99.22% <98.63%> (-0.78%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shyuep
Copy link
Contributor

shyuep commented Oct 2, 2023

I would actually do it by simply add a kwarg called is_directed to create_line_graph which defaults to False. It should be simple to do based on your current implementation.

@lbluque
Copy link
Contributor Author

lbluque commented Oct 3, 2023

@shyuep made the changes you suggested, pls have a look when you can

@shyuep shyuep merged commit 543c3e0 into materialsvirtuallab:main Oct 3, 2023
4 checks passed
@shyuep
Copy link
Contributor

shyuep commented Oct 3, 2023

Thanks

@lbluque lbluque deleted the line_graph branch October 3, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants