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

Towards graph AD for Forces #111

Closed
wants to merge 101 commits into from
Closed

Towards graph AD for Forces #111

wants to merge 101 commits into from

Conversation

DhairyaLGandhi
Copy link
Member

@DhairyaLGandhi DhairyaLGandhi commented Aug 10, 2021

part of closing #10

And its fast, not that FiniteDifferences is supposed to be "fast" but ForwardDiff fails here, and FiniteDifferences is supposed to be correct.

# FiniteDifferences
julia> @time grad(forward_fdm(2,1),
           (i,j,dist) -> sum(GraphBuilding.weights_cutoff(i,j,dist)),
           collect(1:1000), collect(1:1000), Float64.(collect(1:1000)) );
 98.433196 seconds (942.86 k allocations: 270.869 GiB, 20.58% gc time, 4.76% compilation time)

# Zygote
julia> @btime gradient($(collect(1:1000)), $(collect(1:1000)), $(Float64.(collect(1:1000)))) do i, j, dist
         sum(GraphBuilding.weights_cutoff(i, j, dist))
       end;
  7.407 ms (4164 allocations: 31.16 MiB)

thazhemadam and others added 30 commits July 9, 2021 14:51
Define `SimpleCodec` - a very simple and straightforward codec that
represents an encoding-decoding scheme that is purely based upon only
the encoding-decoding functions attached, and the fields of the
`FeatureDescriptor` it is attached to, and doesn't track any parameters
itself.

Signed-off-by: Anant Thazhemadam <[email protected]>
Create a constructor that creates an `ElementFeatureDescriptor` object
given the name of the feature, and the codec associated with the object.

The default `lookup_table` is `atom_data_df`, and the default
categorical value is inferred from the same using the
`default_categorical` helper function.

Signed-off-by: Anant Thazhemadam <[email protected]>
* Minimize repetitive code that would be required to be written while
implementing `encode` and `decode` functionality. All
`FeatureDescriptor`s must have a `Codec` attached. The responsibility of
actually performing the encoding/decoding is delegated to the
`encode_f`/`decode_f` functions of the attached `Codec` respectively.
This would mean that, the definitions for all
`encode(::AbstractFeatureDescriptor, ::AbstractAtoms)` and
`decode(::AbstractFeatureDescriptor, encoded_feature)` functions for all
feature descriptors would look the same.
Also, all `Codec`s must be callable and define an implementation taking
whatever `FeatureDescriptor` they support as the first argument, and
an `AbstractAtoms` object (in case of encoding) or an `encoded_feature`
(in case of decoding) as the second argument (since `encoded_feature`
will no longer be a subtype of `AbstractAtoms`, as of 99618bc).
So, instead of throwing the at the level of the generic `encode`/`decode`,
throw an error (an `ErrorException`) at the level of the generic
callable `Codec` object's syntax, when an invalid combination of
arguments (for which no implementation is present) is executed.

* Modify the test checking for a `MethodError` while performing `decode`
(for `FeatureDescriptor`s) to check for an `ErrorException` instead.

* Remove the `EncodeOrDecode` enum, as it doesn't really serve any real
purpose anymore. The work done to simplify the encoding-decoding logic
in itself renders it unnecessary.
However, regardless of this, `EncodeOrDecode` isn't required anymore.
The `EncodeOrDecode` enum was introduced back when `Atom`s objects used
to be mutated to represent the encoded features as well. However, as of
99618bc, this is no longer the case, and `Atom`s objects are no longer
as tightly coupled to encoded features.
As a result of this, even if an `encoded_features` is essentially an
`Atom`s object that has its values modified, it won't be of a subtype
`AbstractAtom`.
This further means that, all calls being made for `decode` operations
using the callable syntax of a `Codec`, like (`OneHotOneCold`) will be
dispatched using only
`(ed::OneHotOneCold)(efd::ElementFeatureDescriptor, encoded_feature)`, and
`(ed::OneHotOneCold)(efd::ElementFeatureDescriptor, a::AbstractAtoms)`
would be dispatched only for `encode` operations; no longer for
`encode` and possibly `decode` too. This renders the `EncodeOrDecode`
enum unnecessary, as it is not really of any use.

Signed-off-by: Anant Thazhemadam <[email protected]>
* Update script to use the `valence` attribute
* Create and populate a new column for Electronic Strucutre of a given
element in `pymatgen_atom_data.csv`

Signed-off-by: Anant Thazhemadam <[email protected]>
Update `pymatgen_atom_data.csv` dataset using `tabulate_pmg_data.py` to
include new column for `Electronic Structure` for all elements.

Signed-off-by: Anant Thazhemadam <[email protected]>
* Create new `elements` function which will take an `Atoms` object as
argument, and returns the elements present in the `Atoms` object.
This function is useful since it allows for flexibility - every subtype
of `AbstractAtoms` doesn't have to have a field named `elements`.

* In case `elements` is dispatched on an Atoms object whose type doesn't
define an implementation, throw a `MethodError`.

* Update all instances that use the `elements` field of an `AtomGraph`
to use the `elements` function instead.

Signed-off-by: Anant Thazhemadam <[email protected]>
Signed-off-by: Anant Thazhemadam <[email protected]>
`output_shape` is determined using the `Codec` scheme applied on a
`FeatureDescriptor`. Since this will have to be specific to the
`FeatureDescriptor`-`Codec` combination, make the generalized
`output_shape` (which takes only the `FeatureDescriptor` as argument)
generic.

Signed-off-by: Anant Thazhemadam <[email protected]>
* Add comments, and annotate `default_ofd_decode()`

* Turns out, instead of having the electronic configuration for an
element be represented in the order shells are actually filled, the
lookup_table stores them in a more "readable" way, i.e., numerically
sorted way.
For instance, the valence shell configuration for "Fe" is `4s2.3d6` if
you consider the order in which electrons are filled in the shells, but
in the default lookup_table, the order is `3d6.4s2`.
So, sort out the `shell_conf` first, before looking it up.

Signed-off-by: Anant Thazhemadam <[email protected]>
* Slim down the `lookup_table` to have only the `Symbols` and
`Electronic Structure` columns.

* Format using JuliaFormatter

Signed-off-by: Anant Thazhemadam <[email protected]>
The values under the `Electronic Structure` column for Lawrencium
contained "(tentative)" in addition to the configuration. Remove this,
and standardize the configuration value.

Signed-off-by: Anant Thazhemadam <[email protected]>
* Create a new module called `Data`, which is meant to store all the
data structures that represent actual usable data in some form
(DataFrames, JSON, etc).

* Move `atom_data_df` and `feature_info` out of
`Utils.ElementFeatureUtils` into `Data` module.

Signed-off-by: Anant Thazhemadam <[email protected]>
* Export `valenceshell_conf_df` - a DataFrame that contains only
`Symbol` and `Electronic Structure` (valence shell electronic
configuration) as its columns, from the `Data` module.

* Remove the `lookup_table` field from `OrbitalFeatureDescriptor`.
Since the electronic structure is universally consistent in every data
set, instead, simple directly use `valenceshell_conf_df` wherever
`ofd.lookup_table` was being used.

Signed-off-by: Anant Thazhemadam <[email protected]>
Add  "short" and "long" versions of pretty printing for
`OrbitalFeatureDescriptors`.

Signed-off-by: Anant Thazhemadam <[email protected]>
@DhairyaLGandhi
Copy link
Member Author

DhairyaLGandhi commented Sep 15, 2021

Seems like weights_cutoff works still, with a change in sign. So that's great. We'll need neighbours_list to AD as well, and then we would be able to do build_graph. neighbour_list touches a lot of Xtals.jl but I'll have to read into what the function is doing, and going from there.

@DhairyaLGandhi
Copy link
Member Author

Spent a little time updating this locally - sitrep:

julia> c = Crystal(joinpath(cr_path, "IRMOF-1.cif"))
Name: /Users/dhairyagandhi/.julia/packages/Xtals/Kf4en/test/data/crystals/IRMOF-1.cif
Bravais unit cell of a crystal.
	Unit cell angles α = 90.000000 deg. β = 90.000000 deg. γ = 90.000000 deg.
	Unit cell dimensions a = 25.832000 Å. b = 25.832000 Å, c = 25.832000 Å
	Volume of unit cell: 17237.492730 ų

	# atoms = 424
	# charges = 0
	chemical formula: Dict(:Zn => 4, :H => 12, :O => 13, :C => 24)
	space Group: P1
	symmetry Operations:
		'x, y, z'

julia> gradient(c) do c
         i,j,d = neighbor_list(c)
         sum(d)
       end
((name = nothing, box = (a = 1938.7207119336626, b = 1861.6924099523428, c = 1947.283043589588, α = -416.8184235168784, β = 760.7150303623142, γ = -681.7548034041554, Ω = nothing, f_to_c = nothing, c_to_f = nothing, reciprocal_lattice = nothing), atoms = (n = nothing, species = nothing, coords = (xf = [1972.726096133977 -1972.7260961339766  -441.49871345424566 -1138.6631802079703; -986.3630480669881 1972.7260961339766  1922.6874775652561 -1068.3497716550012; -986.3630480669881 -1972.726096133979  9.069665394507859e-14 -854.3377059102552],)), charges = nothing, bonds = nothing, symmetry = nothing),)

julia> gradient(c) do c
         w, s = build_graph(c)
         sum(w)
       end
((name = nothing, box = (a = NaN, b = NaN, c = NaN, α = NaN, β = NaN, γ = NaN, Ω = nothing, f_to_c = nothing, c_to_f = nothing, reciprocal_lattice = nothing), atoms = (n = nothing, species = nothing, coords = (xf = [NaN NaN  NaN NaN; NaN NaN  NaN NaN; NaN NaN  NaN NaN],)), charges = nothing, bonds = nothing, symmetry = nothing),)

Everything works but something becomes NaN at some point. Likely a divide by 0. But we should be able to do forces with this.

@DhairyaLGandhi
Copy link
Member Author

Okay, so we have the following working.

julia> include("xtals.jl")
build_graph2 (generic function with 1 method)

julia> cr_path = normpath(joinpath(pathof(Xtals), "..", "..", "test", "data", "crystals"))
"/Users/dhairyagandhi/.julia/packages/Xtals/Kf4en/test/data/crystals"

julia> c = Crystal(joinpath(cr_path, "IRMOF-1.cif"))
Name: /Users/dhairyagandhi/.julia/packages/Xtals/Kf4en/test/data/crystals/IRMOF-1.cif
Bravais unit cell of a crystal.
	Unit cell angles α = 90.000000 deg. β = 90.000000 deg. γ = 90.000000 deg.
	Unit cell dimensions a = 25.832000 Å. b = 25.832000 Å, c = 25.832000 Å
	Volume of unit cell: 17237.492730 ų

	# atoms = 424
	# charges = 0
	chemical formula: Dict(:Zn => 4, :H => 12, :O => 13, :C => 24)
	space Group: P1
	symmetry Operations:
		'x, y, z'

julia> gradient(c) do c
         w, s = build_graph2(c)
         sum(w)
       end
((name = nothing, box = (a = -31.83244099108285, b = -31.832440991082862, c = -31.832440991082862, α = -2.1514963693844834e-14, β = -2.8035019022818323e-14, γ = -3.751262522266638e-14, Ω = nothing, f_to_c = nothing, c_to_f = nothing, reciprocal_lattice = nothing), atoms = (n = nothing, species = nothing, coords = (xf = [-4.78645247969116 4.786452479691137  51.735173803203864 51.735173803203864; 4.786452479691156 -4.786452479691122  77.23782826208793 -77.23782826208932; 4.786452479691155 4.7864524796911665  -77.23782826208932 77.23782826208792],)), charges = nothing, bonds = nothing, symmetry = nothing),)

which are the gradients for the crystal and graph AD 🎉

Couple of notes:

  1. There were a few missing methods in Xtals to calculate distance, I added those Allow vectors of indices while computing distance SimonEnsemble/Xtals.jl#101 (and also included a copy in the PR)
  2. replicate is written with a lot of mutation, and I figured we could write a simpler version, so I did. How do we want to handle this? We could propose this change to Xtals.jl (the performance is roughly same, but of course this allocates)
  3. I have refactored some of the indexing utilities and removed them from AD to save some time there. It means we can write it in a neater way if desired.
  4. How do we want to test this? Do we have some cases in mind?
  5. I might have messed up the rebase, so I'll have to move this to a different PR.

@DhairyaLGandhi
Copy link
Member Author

Moving to #119

@DhairyaLGandhi DhairyaLGandhi deleted the dg/egs branch September 17, 2021 23:24
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.

6 participants