Refactor into matter

[SimonEnsemble]

This is a huge change.

• Both crystals and molecules are broken down into PtCharge's and LJSphere's. You'll see in the energy functions that this greatly simplifies vdw and electrostatic energy computations since we generally have interactions between to PtCharge's and two LJSphere's for both guest-guest and guest-host interactions.
• Order of arguments in GCMC function are changed to be more intuitive.
• Some speed up, but not much.
• I started writing docs in README.md

After this is merged, we'll make it public and ask others to use it/contribution/post issues for improvements.

Thanks @huynmela for the sweet logo.

Plz @Surluson @huynmela @ahyork help make sure the tests run and point out any bugs.

Premature optimization is the root of all evil. - Donald Knuth

Premature documentation is the root of all evil. - Me

The docs aren't perfect, but the code has been changing so much that honestly it was a waste of time to write the doc strings at first because the code changed so much. I hope it's more stabilized, but I'd like to hold off on perfecting doc strings for now until the code stabilizes. The README.md however let's keep up to date for sure.

[Surluson]

I'm still actively reading through the code, but here's the first part of my review:

high_throughput_helpers/

• jobs_to_run:
  • The pressure has two linspaces, one going from 0 to 1 bar, and the second going from 0 to 20. Shouldn't the second go from 1 to 20 so we're not repeating the 0 to 1 pressure range? :wrench: fixed
• submit_jobs_for_isotherms.jl:
  • For my script I automatically rm'd the submission script after it was submitted in the for-loop. I didn't really try it the other way, so I'm not entirely sure this is a problem. :wrench: sure, I suppose that is good practice in case for some reason the loop fails to overwrite gcmc_submit.sh and then the same job is submitted over and over. But it would be nice to see what the script looks like so we can ensure it is correct!

src/

• Box.jl:
  • I feel like reciprocal_lattice should be able to take in the f_to_c matrix as an input as well, rather than splitting it up into the three unit cell vectors. :wrench: done
• Crystal.jl:
  • Right now the docstrings for Framework are shared between the struct and the previously named read_crystal_structure_file function. This might be intended, but when looking up the function/struct with ?Framework it might be a bit confusing to the user. :wrench: fixed, tried to combine the two.
  • I noticed a mistake on my half from months back with the read_crystal_structure_file function (now named Framework). When checking to see if the cif has P1 symmetry, I check to see if that string is equal to P -1. This is a separate space group (see P1 vs P -1) I'll have to see what effect this has had on everything up until now! :wrench: good catch... I actually might have done that too. P -1 is deleted.
  • I think a function to assign charges to the framework would be nice. Now that when a framework without charges doesn't have Point Charges, having a function that forms the Point Charges objects from an array and adds them to the framework would be nice. :wrench: see assign_charges(framework, charges in Crystal.jl and tests in runtests.jl.
  • I was looking at replicate(framework, repfactors) in Crystal.jl and the assertions at the end of the function. I do think that more assertions are for the better, but aren't those @assert just checking to see if the for-loop ran correctly? I don't see where anything could go wrong in that function. This is really nit-picky by me, but I'd like to hear your thoughts on it. :wrench: yeah that was totally for making sure the loop runs correctly and that I counted the new number of atoms correctly
  • Should we do a charge_overlap function, similar to atom_overlap? I'm not sure it makes any sense, as the point charges don't occupy space. I guess two point charges, not associated with a LJSphere could overlap. The 1/r term would blow up in the electrostatic term though, right? :wrench: good point: I added charge_overlap so now we have remove_atom_and_charge_overlap that we can do, since probably we want to remove that entire atom and charges and atoms comes in pairs typically in a .cif file.

That's all I have for now. I'll be posting the rest later today :+1:

[Surluson]

I don't have many comments on the rest. It looks like most of it is just implementing the changes to each file. There were a few things that I noted down. It is mostly about the Molecules.jl functions and how it affects the other files.

• Molecules.jl/GCMC.jl/NearestImage.jl:
  • I think there should be a safeguard somewhere that people aren't translating the LJSpheres out of bounds. Would adding a mod(xf, 1.0) to the translate! in Molecules.jl introduce some other problems further down the workflow? There are a few functions that I think will break if the LJSpheres aren't within the [0,1] range.
    • In NearestImage.jl: nearest_image! needs correct coordinates to make an accurate nearest image.
    • In MChelpers.jl: apply_periodic_boundary_condition! subtracts or adds 1 to apply the periodic boundary. If someone would translate a molecule by 10 fractional units this wouldn't properly fix it. Either placing a mod(xf, 1.0) somewhere in the workflow, or using mod in this function would fix it. :wrench: true that these functions have strong assumptions. I think here it is a trade-off between speed and safety. For this we have to choose speed I think since that function is only used in the simulation functions that users won't be calling unless they write their own simulation code. I did put a warning in the doc-string, however.
• Molecules.jl: I see that there's a write_to_xyz function and a replicate_to_xyz function in Crystal.jl. Would it make sense to merge these functions? One using a Molecule as an input and the other using a Framework. :wrench: nice that you spotted this. now beautifully write_xyz is overloaded 3 times! less code is more code! and that parallelism :+1:

These functions were spread out between a few files, so it's likely that I missed something. Let me know if you think the translate! out-of-bounds thing is a problem or not.

I still haven't read through the test scripts, that should be ready tomorrow :+1:

[Surluson]

I forgot Forcefield.jl in the last comment.

• Forcefield.jl:

  • missing_atoms should maybe take Array{LJSpheres} as an input instead. It would be easier for the user to write missing_atoms(framework.atoms) than doing some list comprehension. After writing that, I saw that the function is used in check_forcefield_coverage which does have framework/molecule as an input and is very user friendly. So take it or leave it! :wrench: made comment that missing_atoms is a helper for the check coverage functions exposed to user

• REQUIRE:

  • Add ProgressMeter :wrench: done, also put all using X in PorousMaterials.jl instead of having them scattered about.