Closed SimonEnsemble closed 5 years ago
one more thing: the GCMC prints off a bunch of stuff in the beginning.
if snapshots are being taken, can you printf "writing snapshots of adsorption positions every %d cycles (after burn cycles)\n"
if density grid is being written, can you printf "adsorption spatial probability density grid written with spacing %.3f Angstrom every %d cycles (after burn cycles)"
I went through all suggestions and implemented them. The branch passes all tests as well.
This should be ready to merge, I am going to update the docs site so the new function appear there as well.
snapshot_frequency::Int
: is a misleading name for the variable since high frequency implies often. how about snapshot_period::Int
, and it is the period of the snapshots expressed in number of cycles (not samples, which is different since there are many samples in one cycle). would be nice to change sample_frequency
and checkpoint_frequency
to *_period
as well.framework = replicate(framework, repfactors)
since the fractional coordinates will change. This only affects frameworks that are replicated, but still will be a bug for these. so move the snapshots stuff after that line. also plz done with a comment # snapshot / density grid set up
for clarity.close(xyz_snapshot_file)
with new intialization, do u need to close it every time now?num_atoms = 0;
semi-colon not required@test all(xf_to_id(n_pts, [0.0001, 0.24, 0.26]) .== [1, 2, 3])
for another testupdate_density!
? how about:
# test update_density! by placing CO2 strategically to be in voxel (1, 2, 4)
box = UnitCube()
n_pts = (4, 4, 4)
density_grid = Grid(UnitCube(), n_pts, zeros(n_pts...), :inverse_A3, [0.0, 0.0, 0.0])
molecule = Molecule("CO2")
translate_to!(molecule, [0.24, 0.26, 0.99])
update_density!(density_grid, [molecule], :C_CO2)
@test isapprox(sum(density_grid.data), 1.0)
@test isapprox(density_grid.data[1, 2, 4], 1.0)
@printf("\tAdsorption spatial probability density grid written with spacing %.3f Angstroms every %d cycles (after burn cycles)\n", density_grid_dx, snapshot_frequency)
also can you print out density_grid.n_pts
to alert the user if the grid is so fine that it is going to be a huge file?
I am wrapping up with these edits soon, but I was just thinking about changing the *_frequency
to *_period
. This is going to break a lot of backwards compatibility because these are optional arguments. People will need to change their scripts in order to keep this working.
I think it is a good idea to do this, but we should release the next version as 1.0.0 since PorousMaterials.jl is no longer going through rapid development and the API won't be compatible with scripts written for older versions.
I found an issue when writing a test for the O_CO2
in CO2
. One of the atoms is outside the box, and so it cannot be placed at a negative index. I am going to see if I can use NearestImage to get the indices to wrap.
:fireworks: hooray for tests! Didn't think of that...
I think we should leave renaming *_frequency
to *_period
for a later time.
I agree that informative parameter names are important, but that small change breaks some backwards compatibility.
We should leave it out for now, but we should go through and decide what 'stable' PorousMaterials.jl looks like. Then we can fix small errors like this without creating a chain of incompatible releases
sure, that is fine with me!
This should be good to go now! Take a look at the new xf_to_id. It checks to see if it is above and below because it has to treat them with different cases.
thanks @ahyork
molecule.species
which might not even be a species atom; e.g. the grid would be empty for Molecule("CO2")
)xf_to_id!
currently looks like it would modify the coordinates of the molecule? to avoid changing the coordinates of the molecule since arrays are passed by reference, I instead changed it so that periodic boundary conditions are applied to voxel_id
do these changes look ok to you?
Those changes look good to me, I am going to add these new functions to the docs and then merge unless there is anything else you think we should add?
@ahyork
Looks great! Before merging this PR:
gcmc_simulation()
snapshots
to more informativewrite_adsorbate_snapshots
since now it is only writing them to file.xyz_snapshot_file = ""
is initialized as a string. Doesn't that introduce type instability? Doesxyz_snapshot_file = IOStream
work better, initializing an empty IOStream?comment=filename_comment * "adsorbate_positions"
switch order so comment is always at end.if (outer_cycle % snapshot_frequency == 0) && (outer_cycle > n_burn_cycles)
for speed is it better to reverse these? I imagine>
is faster than a%
?write_xyz_to_file
can you change towrite_xyz
to be consistent with the otherwrite_xyz
? i.e. overload it so one less function name to remember.snapshot_frequency=1
by default.snapshot::Bool
: Whether this is the name for a snapshot file" is a vestige of an old version ofgcmc_result_savename
?Grid
object" to be more specific, change to: "returns the indices of the voxel in which it falls when a unit cube is partitioned into a regular grid ofn_pts[1]
byn_pts[2]
byn_pts[3]
voxels.write_xyz_to_file(molecules, xyz_file)
doc string does not match function. specify in doc string it is writing cartesian not fractionalatoms_in_molecules
is redundant given type assertion, how aboutn_atoms(molecules::Array{Molecules})
?update_density!
, the problem here is that for CO2, it would increment the voxel for both C and O then you'd get a density grid that is confusingly keeping track of C and O. I think we should put thespecies
granularity on the atom within the molecule (not the molecule itself), so e.g. the user would pass:C
to keep track of:C
within CO2. soif molecule.atoms.species[i] == species
I think should be the condition for writing