-
Notifications
You must be signed in to change notification settings - Fork 14
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
graph building and file I/O with Xtals.jl #112
Conversation
If this ends up working well, I may just PR that function to Xtals.jl as it could definitely be useful for others, but going to play with it here for now. Signed off by Rachel Kurchin rkurchin@cmu.edu
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Codecov Report
@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 86.97% 87.42% +0.44%
==========================================
Files 17 17
Lines 476 493 +17
==========================================
+ Hits 414 431 +17
Misses 62 62
Continue to review full report at Codecov.
|
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
More details: * Made separate dispatch of build_graph function onto a Crystal object; this should be directly usable as the first layer in a model and differentiate with respect to c.atoms.coords.xf (or convert to Cartesian first) * ASE dependency is no more! The cost of this was loss of compatibility with some structure file formats. Some of these (most notably XYZ) will be recoverable with a pymatgen update. * I removed the normalize_weights option, because I can't conceive of a case where you wouldn't want to do this * I also removed the test for nonperiodicity because I realized this is not generally stored in the structure files but rather in the ASE Atoms object (which has certain defaults when reading in files), so it wasn't an actual information stream Signed off by Rachel Kurchin rkurchin@cmu.edu 25 August 2021
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Co-authored-by: Anant Thazhemadam <47104651+thazhemadam@users.noreply.github.com>
…k for cutoff radius size, change from loops to list comprehensions for neighbor_list Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
… a better longterm solution though Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
@@ -58,7 +58,7 @@ using Xtals | |||
ag2 = deserialize(abspath(@__DIR__, "..", "test_data", "strucs", "testgraph.jls")) | |||
@test adjacency_matrix(ag.graph) == adjacency_matrix(ag2.graph) | |||
@test elements(ag) == ag2.elements | |||
@test Atoms.normalized_laplacian(ag) == ag2.laplacian | |||
@test ChemistryFeaturization.Atoms.normalized_laplacian(ag) == ag2.laplacian |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please expand on what exactly the namespace conflict is? I don't recall seeing any such conflicts while testing with the OFD
PR.
It might also help to actually make a note of it for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's because Xtals also defines Atoms
so while this worked before:
using ChemistryFeaturization.Atoms
# then later...
Atoms.whatever()
that's now ambiguous as to which Atoms
it's referring to, so now I just have
using ChemistryFeaturization
# then later...
ChemistryFeaturization.Atoms.stuff...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. In that case, I can't think of any other long term solution that's better than what we have rn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I mean short of renaming our Atoms
module to something else there isn't really another option
- `max_num_nbr::Integer=12`: maximum number of neighbors to include (even if more fall within cutoff radius) | ||
- `dist_decay_func::Function=inverse_square`: function to determine falloff of graph edge weights with neighbor distance | ||
""" | ||
function build_graph( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a reference to this docstring also under Utilities/Graph Building
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the @autodocs
do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't check right now because my local setup for building the docs is somehow broken again :/ but I thought the @autodocs
block should do it?
|
||
Returns as is, js, dists to be compatible with ASE's output format for the analogous function. | ||
""" | ||
function neighbor_list(crys::Crystal; cutoff_radius::Real = 8.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, can we have a reference to this docstring in the docs? 😄
Signed-off-by: Rachel Kurchin <rkurchin@cmu.edu>
Prerequisite for #10 but won't resolve it on its own. Will also help #111 go more smoothly.
Need to actually implement the neighbor list function, which will be the most intensive part of this. Not really started yet, but I have a clear picture of how it will go and it's sketched out in some comments...