MALBECC / lio

Linear implementation of DFT calculations (CPU and GPU)
GNU General Public License v2.0
26 stars 18 forks source link

Namelist Migration #163

Open ramirezfranciscof opened 6 years ago

ramirezfranciscof commented 6 years ago

We need to implement a new structure for lio keywords:

Why a new namelist?

1) lio namelist is originally defined inside of amber. This makes it extremely cumbersome to modify for new additions. Having a different definition of that namelist in liosolo is not very nice either and though it does not instantly causes problems (since they are defined in different programs), its bugs and complications waiting to happen. 2) Some keywords should probably be replaced for more descriptive ones (fx), or for non reserved words (open). Doing so in the lio namelist directly would not only require to modify amber (if we want to keep doing qmmm), but also would break all previous liosolo inputs.

I propose the following actionable steps:

ramirezfranciscof commented 6 years ago

Regarding this and the recent PR #190 . Right now lio has "3 namelists" and I think all need revision...

The first one and clearest of them all is the new lionml: my intention was to eventually have all keywords be handled here. This is the one where I see the most versatility to work with. However, after seeing the manual I'm begining to wonder if we should have specific namelists for specific configurations, like the gpuoptions that deal with XC calculations criteria. Also: Cubegen is currently part of lio but should probably be better off as a PP program, and Geometry Optimization seems to be some hybrid configuration, doesn't hybrid has its own namelist?

Now, the other two namelists are not clearly separated, but they are the "lio" namelist as read by amber, and the "lio" namelist as read by liosolo. These are not the same and if you put some of the keywords inside the "lio" namelist that is in the input amber, you will get an error (all of the GPU options for example). These namelist non-uniqueness is not ideal and the reason why I push to use a different name for the namelist that is read inside the lib.

All this to say: For the time being, I think it will be best to keep in the manual a clear distinction between the keywords that can be read from amber and the ones that can't. I would also advise to have all input issues sorted before release, but in case we cannot, to have a disclaimer saying that the keyword scheme is soon to be revised and modified. We can keep backwards compatibility though.

@fedepedron thoughts on this?

fedepedron commented 6 years ago

By 3 namelists you mean "lionml", "lio" and "lio"-from AMBER?

Agreed on the fact that maybe a lio namelist which has full compatibility with AMBER should be kept as such, although the new "lionml" should have all options available. Nevertheless, I still do not like the name (though its not that I have a better idea on how to call it...).

Regarding cubegen, maybe it's possible to keep it separate as we keep tdanalize, but geometry optimization is intrinsecally built in LIO (it is not a HYBRID feature, you can actually do restrained minimizations for a full-QM system within LIO) and I believe we should keep it like that.

Regarding the release schedule and for the sake of the (we hope bigger) userbase, these input issues should be sorted out ASAP. And in the manual maybe I would say like "look, AMBER CAN actually read some of the options, but it is better if you keep everything in a separate inputfile", as meaning "this is actually deprecated". This way AMBER/LIO usage is standarized along GROMACS/LIO and (in a future release) LAMMPS/LIO.

PS: On a sidenote, while the error-handling subs for the namelist was initially a good idea, we have the issue that the return error values are not standarized between compiler. Unless you want to take into account every possible scenario, I think these checks should be kept to a minimum.

ramirezfranciscof commented 6 years ago

lio namelist according to MY ambertool file contains:

 character(len=20) :: basis
 character(len=20) :: output
 character(len=20) :: fcoord
 character(len=20) :: fmulliken
 character(len=20) :: frestart
 character(len=20) :: frestartin
 logical :: verbose
 logical :: OPEN
 integer :: NMAX
 integer :: NUNP
 logical :: VCINP
 real*8  :: GOLD
 real*8  :: told
 real*8  :: rmax
 real*8  :: rmaxs
 logical :: predcoef
 integer :: idip
 logical :: writexyz
 logical :: intsoldouble
 logical :: DIIS
 integer :: ndiis
 real*8  :: dgtrig
 integer :: Iexch
 logical :: integ
 logical :: DENS
 integer :: IGRID
 integer :: IGRID2
 integer :: timedep
 real*8  :: tdstep
 integer  :: ntdstep
 logical :: field
 logical :: exter
 real*8  :: a0
 real*8  :: epsilon
 real*8  :: Fx
 real*8  :: Fy
 real*8  :: Fz
 integer  :: NBCH
 integer :: propagator
 logical :: writedens
 logical :: tdrestart

I suspect that at least the last NBCH, propagator, writedens, and tdrestart (and probably ntdstep, tdstep and timedep as well...maybe even DIIS...) are part of the "unofficial" version of sander that only exists in our lab. Maybe these too should go to the new namelist...

fedepedron commented 6 years ago

My recommendation is to check init_lio_amber and start from there.

ramirezfranciscof commented 6 years ago

Nah, I was wrong. Apparently those are the exact same keywords found in the official release of AmberTools.

ramirezfranciscof commented 6 years ago

Keywords for restarts

These should live inside fileio_data module and there should be some functions in fileio (do_rst_write / do_rst_read / do_rst_last or something like that) which receives the criteria number (the number of step or iteration) and returns true or false depending on whether the criteria for writing (or reading) the restart is met. Possibly also return as parameter the unit number so the restart subroutine specific to that part of the code (td, scf, ehrenfest) can write there.

Old namelist compatibility

Obs1: the presence of a non-default frestart turns rstlast true. Obs2: both VCINP and tdrestart turn rstload true, but since td and ehrenfest could both read an SCF restart or initiate propagation from a given density, tdrestart should also turn on a skip_scf keyword while VCINP should turn it off. I'm not sure whether this keyword should live in the fileio_data module with the restart io info, or in the specific data modules of td/ehrenfest...

ramirezfranciscof commented 6 years ago

lio namelist according to what lio can read:

# General:
 integer :: natom
 integer :: nsol
 integer :: charge
 logical ::  OPEN
 integer :: Nunp
 integer :: NMAX
 integer :: lowdin
 integer :: verbose
 real*8  :: rmax
 real*8  :: rmaxs
 logical :: int_basis
 character(len=20) :: basis_set
 character(len=20) :: fitting_set
 logical :: predcoef
 logical :: intsoldouble
 real*8 :: dgtrig
 integer :: Iexch
 logical :: integ
 logical :: dens
 integer :: igrid
 integer :: igrid2
 double precision :: good_cut
 logical :: writedens
 logical :: Dbug
 integer :: timers
 logical :: gaussian_convert

# GPU Optim:
 real*8 :: little_cube_size
 integer :: max_function_exponent
 real*8 :: free_global_memory
 integer :: min_points_per_cube
 logical :: assign_all_functions
 real*8 :: sphere_radius
 logical :: remove_zero_weights
 logical :: energy_all_iterations

# SCF Convergence:
 logical :: hybrid_converg
 logical :: DIIS
 integer :: ndiis
 real*8  :: GOLD
 real*8  :: told
 real*8  :: Etold

# Input / Output:
 integer :: style
 integer :: idip
 integer :: writexyz
 logical :: fukui
 logical :: mulliken
 logical :: VCINP
 integer :: restart_freq
 integer :: td_rst_freq
 character(len=20) :: frestart
 character(len=20) :: frestartin
 logical :: allnml
 logical :: dipole
 logical :: writeforces
 logical :: print_coeffs

# Electron Dynamics
 integer :: timedep
 integer :: propagator
 integer :: NBCH
 logical :: tdrestart
 real*8  :: tdstep
 integer :: ntdstep

# Transport
 logical :: transport_calc
 logical :: generate_rho0
 logical :: gate_field
 integer :: save_charge_freq
 real*8 :: driving_rate
 integer :: Pop_Drive

# DFTB
 logical :: dftb_calc
 integer :: MTB
 real*8 :: alfaTB
 real*8 :: betaTB
 real*8 :: gammaTB
 real*8 :: Vbias_TB
 integer :: end_bTB
 integer :: start_tdtb
 integer :: end_tdtb
 logical :: TBsave
 logical :: TBload

# External Field
 logical :: field
 real*8  :: Fx
 real*8  :: Fy
 real*8  :: Fz
 real*8  :: epsilon
 real*8  :: a0
 integer :: nfields_iso
 integer :: nfields_aniso
 character(len=20) :: field_iso_file
 character(len=20) :: field_aniso_file

 # ECP
 logical :: ecpmode
 character(len=30) :: tipeECP
 integer :: ecptypes
 integer, dimension(128) :: ZlistECP
 logical :: cutECP
 logical ::  ecp_debug
 integer :: verbose_ECP
 logical :: FOCK_ECP_read
 logical :: FOCK_ECP_write
 logical :: ecp_full_range_int
 integer :: local_nonlocal
 logical :: Fulltimer_ECP
 double precision :: cut2_0, cut3_0

# Geometry optim (?):
 integer :: number_restr
 logical :: steep
 real*8 :: Force_cut, Energy_cut
 real*8 :: minimzation_steep
 integer :: n_min_steeps
 logical :: lineal_search
 integer :: n_points

# Cube Gen (?):
 logical :: cubegen_only
 integer :: cube_res
 integer :: cube_sel
 logical :: cube_dens
 character(len=20) :: cube_dens_file
 logical :: cube_orb
 character(len=20) :: cube_orb_file
 logical :: cube_elec
 character(len=20) :: cube_elec_file
 logical :: cube_sqrt_orb

IDK about you, but just looking at this it seems like a freaking mess: just putting it together was a pita, it's almost impossible to know what a keyword does based on the name, or even what part of the calculation it could affect, or where it is initialized to its default. There is even one variable named "integ": I dare every one of you to try to grep your way to finding out what it does.

There are exeptions to all of this, of course: for example, DFTB and transport have all variables declared and initialized in the same place. But still, the general picture is caos, specially on the user end. I think we should do something about how we are organizing and dealing with keywords before this keeps getting further out of hand.

fedepedron commented 6 years ago

For the record, integ, dens, dgtrig, intsoldouble, idip, and similar others are gone as of #235.

I agree that this is quite a mess, and I liked @gonzalodm 's proposal to keep different namelists for different purposes (e.g., &tddft for TD variables or &dftb for DFTB variables).

ramirezfranciscof commented 6 years ago

For the record, integ, dens, dgtrig, intsoldouble, idip, and similar others are gone as of #235.

God bless the Pedron. BTW: I have no idea how you managed to get rid of integ. Or at least to make sure you got rid of it.

I agree that this is quite a mess, and I liked @gonzalodm 's proposal to keep different namelists for different purposes (e.g., &tddft for TD variables or &dftb for DFTB variables).

In which case we need to start discussing the divissions that would make more sense. And what variables need to go/change.