simonensemble / porousmaterials.jl

Julia package towards classical molecular modeling of nanoporous materials

Documentation	DOI	Build Status	Test Coverage

A pure-Julia package for classical molecular modeling of adsorption in porous crystals such as metal-organic frameworks (MOFs).

porousmaterials.jl's Issues

updated REQUIRE

@ahyork for your switch to 0.7 branch, when I:

Pkg.add(PackageSpec(url="https://github.com/SimonEnsemble/PorousMaterials.jl", rev="switch_to_0.7.0"))

the REQUIRE file is not up to date with JLD2 and OffsetArrays

3D Histogram of Molecule Simulations

Requires molecular position data from issue 104. Creates a 3D histogram using the grid datatype to visualize a cloud.

migrate to GitLab CI

Travis CI is dead (to us). Long live GitLab CI!

no parallelism in stepwise adsorption isotherm

stepwise_adsorption_isotherm runs fully serially, making it very slow. This causes gcmc_long.jl to run much longer than it otherwise would for the Xenon and CO2 tests when using multiple threads.

The comments in gcmc_long.jl suggest that the CO2 tests should run in parallel, but this is not the case; the only parallel GCMC function is adsorption_isotherm, which uses pmap to distribute calls to μVT_sim; stepwise_adsorption_isotherm calls μVT_sim in a serial for-loop.

Simulation with two boxes

Is there a way to use PorousMaterials.jl to carry out simulation with two boxes and allow particles within two boxes to swap, as in Gibbs ensemble simulation?

CIF Symettry issues

The second and forth CIF are throwing errors when put into Porous Materials. It has to do with reading symmetry operations.
https://pubs.acs.org/doi/10.1021/jacs.9b05872
@ahyork

codecov

https://github.com/julia-actions/julia-runtest

https://github.com/julia-actions/julia-processcoverage

https://github.com/julia-actions/julia-uploadcodecov

Outputing results to terminal

This is a newbie question but I am trying to detect overlapping atoms using PorousMaterials.jl

Thanks to PorousMaterials.atom_overlap(framework, overlap_tol=0.1) function that I can do this quickly for a large number of structures. However, the output is only shown if I am in Julia environment.

Is there a way to output "CrystalStructureName TRUE/FALSE" for a given CIF If I would like to run below Julia command in terminal? Below is a sample output that I want.

>> julia -p 4 script.jl HKUST-1.cif
HKUST-1 false
>> 

cif reader

make _atom_site_label or atom_type_site_label optional of which col to read in for MOF atoms.

IAST

any value in adding IAST calculations via pyIAST?

Would make predicting gas sensing/separations on hypothetical materials pretty streamlined.

dealing with .cif issues, overlapping atoms

• add types to exclude_multiple_labels, more explanation in docs and more intuitive name
• print a warning if the atoms are merged in a .cif file, which atoms are merged, print framework.name too for high-throughput
• before you merge the two atoms, make sure they are indeed overlapping; if they are not overlapping then throw an error and refuse to read in the framework b/c something is
  wrong with the assumption about the .cif atom label convention.
• maybe instead of merging atoms based on their names in .cif, merge based on whether or not they are overlapped? You know better than me since you did more research into what those column names mean and the atom naming conventions in .cifs. I think fine as is as long as you do above and check they are overlapping before you merge, print a warning, and add in the documentation that this is what you do automatically.

suggestions for PorousMaterials.jl

Hello,

I think the code is well-documented and will help others to run/deploy monte carlo simulations with ease. However, I have a couple of suggestions for the code.

1. I cannot help notice there are some special characters, e.g., α for alpha angle in crystal, everywhere in the code. I think it makes much easier to type in "alpha" instead of copy and paste "α", or volume instead of "Ω". Another example is "π". I find it much easier to simply type "pi" than copy/paste "π " and I am sure other people feel the same way. So perhaps it might be good to change these special characters into words.

2. People who will test drive the code will be the ones that will be much comfortable running the code in terminal environment and not necessarily in Julia environment. So it would be nice if the examples are converted into command line scripts.

Thanks and I hope the code will be a success!!

Optimizing `vdw_energy()`

I think r_squared follows from dx as r_squared = sum(dx .* dx), no? i.e., no need to compute x_k or do molecule.pos[:,i] - x_k. Is that right?

use `@views` for efficiency

there are parts of the code that could benefit from using @views

DOCUMENTER_KEY missing from docs deployment in CI

add to deploy-docs:

env:
      GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}
      DOCUMENTER_KEY: ${{secrets.DOCUMENTER_KEY}}

error during importing crystal structure

I am getting the following error while importing a CIF file.

julia> framework = Framework("FIQCEN_clean_min_charges.cif")
ERROR: MethodError: Cannot convert an object of type String to an object of type PorousMaterials.Framework
This may have arisen from a call to the constructor PorousMaterials.Framework(...),
since type constructors fall back to convert methods.
Stacktrace:
[1] PorousMaterials.Framework(::String) at ./sysimg.jl:77

package dependency woes?

dev PorousMaterials gives:

  Updating registry at `~/.julia/registries/General`
  Updating git-repo `https://github.com/JuliaRegistries/General.git`
  Updating git-repo `https://github.com/SimonEnsemble/PorousMaterials.jl.git`
 Resolving package versions...
ERROR: Unsatisfiable requirements detected for package JLD [4138dd39]:
 JLD [4138dd39] log:
 ├─possible versions are: [0.5.0-0.5.9, 0.6.0-0.6.11, 0.8.0-0.8.3] or uninstalled
 ├─restricted to versions 0.0.0-* by PorousMaterials [68953c7c], leaving only versions [0.5.0-0.5.9, 0.6.0-0.6.11, 0.8.0-0.8.3]
 │ └─PorousMaterials [68953c7c] log:
 │   ├─possible versions are: 0.1.2 or uninstalled
 │   └─PorousMaterials [68953c7c] is fixed to version 0.1.2+
 └─restricted by julia compatibility requirements to versions: uninstalled — no versions left

Research on symbols versus strings

Dataframes.jl uses symbols to query a column. There may be speed advantages to using symbols instead of strings. See here.

Since we're calling ljforcefield.epsilon[atom_i][atom_j] millions of times, I'm wondering if it would speed up computations if we read in framework.atoms as an array of symbols and used symbols as keys for calling force field parameters.

@time will be helpful for this.

split `runtests.jl` suite into multiple files

(@ahyork plz for when summer starts!)
current the runtests.jl is a mess:

• lack of comments (some are my fault but we should try to justify the tests)
• each test suite should be independent and moved to a different file e.g. crystal_tests.jl
• there are many dependences where e.g a framework is defined way up top, not used for a while, but then used again at the bottom. partitioning the tests into modules for each test suite will help fix this. but each test should be as independent and modular as possible.
• how to get Pkg.test("PorousMaterials") to run the tests instead of cd'ing into the test directory and running the manually? i.e. we would like the tests to interface with the julia package manager eventually
• GCMC tests should still be separate since these take super long to run.

visualizations

replace Jupyter notebooks in viz/ with Pluto code, possibly using Xtals v0.3.2

If atomic mass of a molecule is zero, its center of mass is NaN

Check that center of mass is not NaN when returning a molecule after constructing it?
Change center of mass function so that if the mass is zero, just the geometric center of mass is used?

doctests

make code examples in docs into jldoctest where possible

custom path when running PorousMaterials.jl

Is there a way for PorousMaterials to FIRST look at the filenames where the program is being executed THEN look at default PorousMaterials.jl data/crystals/ directory? For example, some people would like to run calculations on the folder which contains both script and structure files.

Below is an error message that I received when trying to run one of the example calculations.

using PorousMaterials

framework = Framework("CAXVII_clean.cif")

molecule = Molecule("CO2")
set_fractional_coords!(molecule, framework.box)

translate_to!(molecule, [0.0, 1.0, 0.0], framework.box)

rotate!(molecule, framework.box) # let's give it a random orientation

eparams, kvectors, eikar, eikbr, eikcr = setup_Ewald_sum(12.0, framework.box)

energy = electrostatic_potential_energy(framework, molecule, eparams, kvectors, eikar, eikbr, eikcr)

$ julia -p 4 test.jl
ERROR: LoadError: SystemError: opening file /Users/ygchung/Desktop/git/CoRE-MOFs/internal/data/crystals/./DOTSOV_clean.cif: No such file or directory
Stacktrace:
[1] #systemerror#44 at ./error.jl:64 [inlined]
[2] systemerror(::String, ::Bool) at ./error.jl:64
[3] open(::String, ::Bool, ::Bool, ::Bool, ::Bool, ::Bool) at ./iostream.jl:104
[4] open(::String, ::String) at ./iostream.jl:132
[5] #Framework#4(::Bool, ::Float64, ::Bool, ::Bool, ::Type{T} where T, ::String) at /Users/ygchung/Desktop/git/PorousMaterials.jl/src/Crystal.jl:46
[6] PorousMaterials.Framework(::String) at /Users/ygchung/Desktop/git/PorousMaterials.jl/src/Crystal.jl:40
[7] include_from_node1(::String) at ./loading.jl:576
[8] include(::String) at ./sysimg.jl:14
[9] process_options(::Base.JLOptions) at ./client.jl:305
[10] _start() at ./client.jl:371
while loading /Users/ygchung/Desktop/git/CoRE-MOFs/internal/test.jl, in expression starting on line 3

docs for energy minimization

• add doc string to the grid search function
• simplify/update doc string for the energy min function

Reinsert move in GCMC

Choose a molecule already present in the system at random, reinsert it at a different position and with a different orientation.

Only actually attempt if length(molecules)>0 of course, as in our delete move.

CIF reader interprets whitespace lines as actual data

When reading in symmetry rules it reads a line with only whitespace as another symmetry rule because it contains characters but they are not actual data
This might appear in other sections, but unsure

code coverage

how to get a code coverage badge to identify functions/snippets of codes that are not touched by a test in runtests.jl?

codecov

codecov seems to be behaving incorrectly... every report claims 0% base and diff coverage, despite CI tests running.

delete main data folder

test/data is up to date. data is not. we should remove data so we don't need to maintain two different data folders.

add framework name to `results` in GCMC function

so we can assert what crystal the simulation was conducted in

henry `autosave`

@ngantzler the Henry code runs till end but then fail to save results. get error: ERROR: LoadError: KeyError: key :sims not found. I think this is b/c u need to mkdir() the directory where results are stored. think this should be automatic. if it doesn't exist, make the directory. otherwise run risk of running tons of sims and then not writing the results.

Crystal reader can't deal with files where there are numbers in the first column of _atom loop

PorousMaterials fails to read the following cif file.
From a quick glance, it looks like the reader assumes atom labels are in the first column in the _atom loop.

Some cif files only contain the atom_site_label tag (in addition to the coordinates) so it's not as easy as only looking at one specific column.

Thoughts how to deal with this?

data_C132H126N12O18_2018-02-15_11:45:50
#******************************************
#
# CIF file created by Zeo++
# Zeo++ is an open source package to
# analyze microporous materials
#
#*******************************************

_cell_length_a      30.8683   
_cell_length_b      30.8684   
_cell_length_c      35.0319   
_cell_angle_alpha       90   
_cell_angle_beta        90   
_cell_angle_gamma       120   

_symmetry_space_group_name_H-M      'P1'
_symmetry_Int_Tables_number     1   
_symmetry_cell_setting      Monoclinic

loop_
_symmetry_equiv_pos_as_xyz
'+x,+y,+z'

loop_
_atom_site_label
_atom_site_type_symbol
_atom_site_fract_x
_atom_site_fract_y
_atom_site_fract_z
1   C   0.502983    0.988842    0.009974
2   C   0.501284    0.956851    0.039354
3   C   0.465216    0.905491    0.039245
4   C   0.429763    0.886817    0.009378
5   C   0.430024    0.918239    0.97983
6   C   0.467501    0.969506    0.980015
7   H   0.527016    0.971118    0.060998
8   H   0.403333    0.849619    0.009162
9   C   0.541625    0.039467    0.010572
10  C   0.540608    0.074255    0.036399
11  C   0.580923    0.124471    0.037492
12  C   0.621053    0.138657    0.011623
13  C   0.622595    0.104336    0.985773
14  C   0.582884    0.054506    0.986044
15  H   0.649788    0.174972    0.011506
16  H   0.583779    0.02903 0.967769
17  O   0.467179    0.001084    0.952894
18  O   0.498429    0.059384    0.057606
19  H   0.493664    0.007029    0.934514
20  H   0.501097    0.041448    0.079872
21  C   0.664327    0.119912    0.958184
22  H   0.681599    0.096708    0.962089
23  H   0.693343    0.158636    0.963884
24  C   0.583509    0.164241    0.063369
25  H   0.57979 0.191077    0.045295
26  H   0.620658    0.183867    0.076563
27  C   0.465103    0.872576    0.070308
28  H   0.460975    0.88733 0.097619
29  H   0.433034    0.834776    0.067247
30  C   0.391628    0.897243    0.949104
31  H   0.373876    0.92014 0.946218
32  H   0.361987    0.859607    0.957005
33  C   0.054578    0.5082  0.67664
34  C   0.086569    0.538492    0.706022
35  C   0.13793 0.553784    0.705911
36  C   0.156603    0.537003    0.676044
37  C   0.125179    0.505843    0.646496
38  C   0.073915    0.492055    0.646682
39  H   0.072303    0.549957    0.727665
40  H   0.193801    0.547773    0.67583
41  C   0.003954    0.496218    0.67724
42  C   0.969166    0.460412    0.703065
43  C   0.918951    0.45051 0.704158
44  C   0.904762    0.476452    0.67829
45  C   0.939087    0.512318    0.65244
46  C   0.988917    0.522437    0.652711
47  H   0.868449    0.468877    0.678173
48  H   0.014389    0.548805    0.634436
49  O   0.042336    0.460154    0.619561
50  O   0.984036    0.433105    0.724274
51  H   0.036392    0.480694    0.601181
52  H   0.001972    0.453709    0.74654
53  C   0.923511    0.538473    0.62485
54  H   0.946712    0.578948    0.628755
55  H   0.884787    0.528766    0.630551
56  C   0.87918 0.413328    0.730035
57  H   0.852342    0.38277 0.711963
58  H   0.859552    0.430849    0.743228
59  C   0.170845    0.586587    0.736977
60  H   0.156089    0.567703    0.764286
61  H   0.208645    0.592317    0.733914
62  C   0.146177    0.488445    0.61577
63  H   0.123279    0.447795    0.612884
64  H   0.183812    0.496439    0.623672
65  C   0.535221    0.540437    0.343307
66  C   0.504928    0.542136    0.372686
67  C   0.489636    0.578204    0.372578
68  C   0.506416    0.613658    0.34271
69  C   0.537578    0.613396    0.313163
70  C   0.551367    0.575922    0.313349
71  H   0.493463    0.516403    0.394332
72  H   0.495647    0.640087    0.342496

energy grid not in docs

documentation for open-sourcing

I'd like to use this theme 😃
See their docs here.
Documenter.jl can be used to automatically generate docs using the doc strings in our code.

Efficiency in `vdw_energy`?

The developer of https://github.com/m3g/CellListMap.jl (@lmiq / @m3g) points out that vdw_energy is potentially inefficient if applied repeatedly to collections of unchanging coordinates. It may be worth working these CellListPair objects in to the simulation code, such that pairwise distances are updated on-demand. Food for thought.

TagBot trigger issue

This issue is used to trigger TagBot; feel free to unsubscribe.

If you haven't already, you should update your TagBot.yml to include issue comment triggers.
Please see this post on Discourse for instructions and more details.

If you'd like for me to do this for you, comment TagBot fix on this issue.
I'll open a PR within a few hours, please be patient!

Error When Trying to Write Adsorbate Snapshots

When setting write_adsorbate_snapshots=true to μVT_sim you get the following error:

ERROR: LoadError: MethodError: no method matching write_xyz(::Box, ::Array{Molecule{Frac},1}, ::IOStream)
Stacktrace:
[1] μVT_sim(::Crystal, ::Molecule{Cart}, ::Float64, ::Float64, ::LJForceField; molecules::Array{Molecule{Cart},1}, n_burn_cycles::Int64, n_sample_cycles::Int64, sample_frequency::Int64, verbose::Bool, ewald_precision::Float64, eos::Symbol, autosave::Bool, show_progress_bar::Bool, write_adsorbate_snapshots::Bool, snapshot_frequency::Int64, calculate_density_grid::Bool, density_grid_dx::Float64, density_grid_species::Nothing, density_grid_sim_box::Bool, results_filename_comment::String) at /home/ng/.julia/dev/PorousMaterials/src/gcmc.jl:542
[2] top-level scope at /home/ng/M-SDB/take_adsorption_snapshots.jl:14
[3] include(::Function, ::Module, ::String) at ./Base.jl:380
[4] include(::Module, ::String) at ./Base.jl:368
[5] exec_options(::Base.JLOptions) at ./client.jl:296
[6] _start() at ./client.jl:506

I believe this is caused by a typo here:

PorousMaterials.jl/src/gcmc.jl

Line 200 in 14ad7c1

xyz_snapshot_file = open(xyz_snapshot_filename, "w")

should be xyz_snapshots_filename?

`fit_adsorption_isotherm` attempts to mutate our dataframe we pass in

fit_adsorption_isotherm(xe_isotherms[mof], common_pressure_units, common_loading_units, :langmuir) results in:

ArgumentError: CSV.Column is read-only; to get a mutable vector, do `copy(col)` or to make all columns mutable do `CSV.read(file; copycols=true)` or `CSV.File(file) |> DataFrame`

I think it is dangerous to mutate the dataframe inside the function. @Surluson could you please address this with a PR? Please show @ngantzler how you fixed it so he can learn some Julia stuff. Thanks!

change FIGlet font

The current FIGlet font for the test script banner is ugly. Choose a new one.

doc website

@ahyork thanks for your work on this, looks great! what is the address of the docs page?

TODO

• put doc badge on README.md
• cat the doc strings after the tutorial on the same page instead of having a separate page for them on the site under each header

@ahyork no rush at all.

Visualization of Molecules

Take snapshots of molecule locations in unit cell during GCMC simulations.

Paths and Windows-compatibility

Hi SimonEnsemble!

It seems that all the paths in PorousMaterials.jl are constructed via string concatenation. For greater inter-platform compatibility (notably, to enable compatibility with Windows machines), it's probably better to be constructing paths via joinpath, or a similar command. In my fork, I've searched through the code for all instances of path construction and have modified them with joinpath statements, so now it runs on my Windows machine (with all tests passing). If this of interest, I'll open a PR once I double-check I got them all. If not, feel free to close the issue.

more informative error for cif not in P1 symmetry

f = Framework("non_P1.cif")

gives an uninformative error message:

ERROR: AssertionError: occursin("P1", line[2] * line[3]) || (occursin("P 1", line[2] * line[3]) || occursin("-P1", line[2] * line[3]))
Stacktrace:
 [1] #Framework#4(::Bool, ::Float64, ::Bool, ::Bool, ::Type, ::String) at /home/simoncor/.julia/dev/PorousMaterials/src/Crystal.jl:73
 [2] Framework(::String) at /home/simoncor/.julia/dev/PorousMaterials/src/Crystal.jl:40
 [3] top-level scope at none:0

can we change to something like "PorousMaterials.jl does not currently support .cif files not in P1 symmetry. Convert your .cif to P1 symmetry using..." and point them to a resource (is it open babel you use @Surluson ?)

add `save_results::Bool=true` default argument to `gcmc_simulation`

use JLD.jl to automatically save to DATA_DIR * "/sim_results". See the root filename function to keep a rigid savename convention. Then you can load them in easily later without trying to recall the filename.

tag version

@ahyork you're right that it'd be better to start tagging a version now so we can ask "what version of PorousMaterials.jl are you on?". it helps for debugging. We can change the version every time there is a pull request? We can start with v0.0.1?

Using `length` on framework.charges and molecule.charges

In line 172 of Grid.jl, there is the following statement:

charged_system = (length(framework.charges) > 1) && (length(molecule.charges) > 1)

I receive the error below regarding the use of length on framework.charges or molecule.charges:

ERROR: MethodError: no method matching length(::Charges)
Closest candidates are:
  length(::Core.SimpleVector) at essentials.jl:571
  length(::Base.MethodList) at reflection.jl:728
  length(::Core.MethodTable) at reflection.jl:802
  ...
Stacktrace:
 [1] top-level scope at none:0

Should line 172 not be the following instead? Using the updated line results in the successful generation of energy grids:

charged_system = (framework.charges.n_charges > 1) && (molecule.charges.n_charges > 1)

calculate_ϕ

The code for calculate_ϕ seems off. z and ρ are calculated, but never used.

run CI build on PRs

CI pipeline runs on pushes, but not on PRs. Should set that up?

set_paths

incorporate use of set_paths from Xtals v0.3.1

grid replication?

what is the feasibility of loading energy/density grids from file and applying replicate(grid, repfactors)?