votca / xtp

GW-BSE for excited state Quantum Chemistry in a Gaussian Orbital basis, electronic spectroscopy with QM/MM, charge and energy dynamics in complex molecular systems
29 stars 15 forks source link

Options refactor #704

Closed JensWehner closed 3 years ago

JensWehner commented 3 years ago

The beginning of the options refactor.

closes #704

JensWehner commented 3 years ago

@votca-bot format

JensWehner commented 3 years ago

@votca-bot changelog: overhaul complete option handling

rubengerritsen commented 3 years ago

I have started with doing a simple dftgwbse. When I generate the options with

xtp_tools -p dftgwbse -o dftgwbse.xml

I get the following output

Writing options for calculator dftgwbse to dftgwbse.xml
nothing to be done - stopping here

Which reads like an error message (I know it isn't) which is confusing, because it had something to do and it even did it.

rubengerritsen commented 3 years ago

I don't know how much control there is over the ordering of the options in the -p generated xml files, but it is confusing that some options are so far from one another. Two examples the package name and package executable are separated by 5 lines and the functional and basisset options are not close either.

rubengerritsen commented 3 years ago

It also seems that there are still some options that could be hidden from users completely like the

options

rubengerritsen commented 3 years ago

When you use orca to do the dft calculation it crashes now. It can't parse the functional.

To reproduce use the defaults and run a dftgwbse calculation on any molecule.

JensWehner commented 3 years ago

Thanks. I will fix the orca problem.

Concerning hiding the options, they have sensible defaults now, so the user does not have to modify them and they are effectively hidden. We could hardcode them, but I would do that in a new PR later down the line.

JensWehner commented 3 years ago

concerning the nothing to be done - stopping here you are right, but it is in tools::application and I do not want change that at the moment, because I do not want to touch csg as well. I agree with you in principle.

baumeier commented 3 years ago

It also seems that there are still some options that could be hidden from users completely like the

  • scratch
  • cleanup and
  • temporary files (this is just an option that names a temporary file temp, that could be hard coded I think, the name of the file shouldn't matter it is temporary)

options

Regarding the scratch and temp options: if I am not mistaken, they were mainly used on cluster machines to move some IO intensive steps (like orca's scratch files) from network drives to fast local disks, no? Why do we have two?

JensWehner commented 3 years ago

What do you mean with

Why do we have two?

? Scratch is the directory and temporary file is the filename used for the files. We could isolate all of this if we remove the input,dft,parse and turn it into dft and encapsulate the qmpackages much more.

baumeier commented 3 years ago

I wasn't sure anymore that scratch -> directory and temp -> filename.

rubengerritsen commented 3 years ago

@JensWehner can you fix the bad merge in your latest commit and then merge this in master?

JensWehner commented 3 years ago

@votca-bot format

JensWehner commented 3 years ago

I cannot merge it into master yet, because for that we have to fix the documentation.

JensWehner commented 3 years ago

@votca-bot format

JensWehner commented 3 years ago

@votca-bot format

codecov[bot] commented 3 years ago

Codecov Report

Merging #704 (fd5288c) into master (b677c32) will increase coverage by 0.2%. The diff coverage is 28.7%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #704     +/-   ##
========================================
+ Coverage    51.4%   51.6%   +0.2%     
========================================
  Files         293     289      -4     
  Lines       27229   27060    -169     
========================================
- Hits        14006   13988     -18     
+ Misses      13223   13072    -151     
Impacted Files Coverage Δ
include/votca/xtp/bsecoupling.h 100.0% <ø> (ø)
include/votca/xtp/dftengine.h 100.0% <ø> (ø)
include/votca/xtp/esp2multipole.h 0.0% <ø> (ø)
include/votca/xtp/gwbseengine.h 0.0% <ø> (ø)
include/votca/xtp/jobcalculator.h 0.0% <0.0%> (ø)
include/votca/xtp/jobtopology.h 22.2% <ø> (ø)
include/votca/xtp/parallelxjobcalc.h 0.0% <ø> (ø)
include/votca/xtp/polarregion.h 0.0% <0.0%> (ø)
include/votca/xtp/qmcalculator.h 0.0% <0.0%> (ø)
include/votca/xtp/qmpackage.h 31.5% <0.0%> (+1.5%) :arrow_up:
... and 83 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b677c32...fd5288c. Read the comment docs.