search

SimonEnsemble / PorousMaterials.jl

Julia package towards classical molecular modeling of nanoporous materials
GNU General Public License v3.0
50 stars 11 forks source link

refactored grid functions to take grid_dx input instead of n_pts #167

Closed eahenle closed 3 years ago

eahenle commented 3 years ago

@SimonEnsemble this will be a breaking-change for existing energy-grid and accessibility scripts. could refactor to allow n_pts arg as alternative to grid_dx to allow backward compatibility

SimonEnsemble commented 3 years ago

I think we shld keep n_pts as an argument, e.g. for ML we might want fixed-size grids. I was thinking just to make the required_n_pts exposed and put that in the grids documentation. then the user has to call that if they want a certain dx.

oh another elegant option is to axe n_pts and change it to: resolution::Union{Float64, Tuple{Int, Int, Int}}. if isa(resolution, Float64) dx=resolution then computes n_pts from dx. if isa(resolution, Tuple{Int, Int, Int}) n_pts=resolution and proceed.

eahenle commented 3 years ago

required_n_pts actually was already exposed and documented. Implementing resolution as suggested.

eahenle commented 3 years ago

@SimonEnsemble ready to merge

SimonEnsemble commented 3 years ago

CC @ngantzler the energy_grid function now takes in an argument resolution instead of n_pts. can be Float to specify resolution or a tuple to specify number of points