Reworked Default/Optional XML #413

baumeier · 2020-03-31T18:10:30Z

Very first attempt at getting new default xml file behavior, testing on dftgwbse calculator:

full dftgwbse.xml file in share folder
calculator passes dftpackage options and gwbse options explicitly through, instead of the xml files
xml file reading only on calculator level

Still not good:

basisset, functional definitions are repeating
some tags are superfluous
xtp_tools still needs to be passed an user option file on the command line, which can be empty though

…tgwbse calculator: - full dftgwbse.xml file in share folder - calculator passes dftpackage options and gwbse options explicitly through, instead of the xml files - xml file reading only on calculator level Still not good: - basisset, functional definitions are repeating - some tags are superfluous - xtp_tools still needs to be passed an user option file on the command line, which can be empty though

ghost · 2020-03-31T18:10:40Z

DeepCode failed to analyze this pull request

Something went wrong despite trying multiple times, sorry about that.
Please comment this pull request with "Retry DeepCode" to manually retry, or contact us so that a human can look into the issue.

- basisset, auxbasisset, functional now given on base level - dftgwbse.cc then transfers this into to both DFT and GWBSE options - reporting removed, was not used at all - archive file name now derived from XYZ file name - XML output summary name now derived from XYZ file name

JensWehner · 2020-04-01T20:19:18Z

I am not sure if all options should have a default, i.e. xyz file. Why do you want to default everything? I mean we can also put the xyz input into the xml file, if we want to reduce the number of files

baumeier · 2020-04-01T20:40:28Z

The xyz input default does not hurt anyone. Adding the coordinates into the xml file is something I don't like. What people will always have is a coordinate file in most likely that format. Asking them to add xml tags around it is just going to be annoying for people.

And why defaulting everything? Just consider the minimum input file needed for orca
* xyzfile 0 1 system.xyz
It will do HF with def2-SVP.

JensWehner · 2020-04-02T07:13:18Z

so orca does not default on the system.xyz as votca will do now.

baumeier · 2020-04-02T07:24:04Z

Yes. We can "undefault" the xyz file name of course since a calculator as defined way down in tools or rather the application that xtpapplicaton is derived from strictly requires an optionfile to be supplied. Now in the full default case that would be a ridiculously looking thing like

<options>
<dftgwbse/>
</options>

If we require xyzfile to be specified, at least it will make sense to require the options file:

<options>
<dftgwbse>
<molecule>system.xyz></molecule>
</dftgwbse>
</options>

removed mo and log file options from gwbse engine

…ults-xml

Felipe defaults xml

…ults-xml

junghans · 2020-04-09T18:11:37Z

Merge after votca/votca#263

CHANGELOG.md

junghans · 2020-04-09T21:44:28Z

It makes one test fail:

 230/244 Test #230: unit_test_orca .....................................***Failed    0.05 sec
 Running 5 test cases...
 Warning: VOTCASHARE environment variable not defined
 /builds/votca/votca/votca/xtp/src/tests/test_orca.cc(8634): error: in "orca_test/input_generation_version_4_0_1": check inp.substr(index1, index2 - index1) == "%basis\nGTOName =\"system.bas\";\nGTOAuxName =\"system.aux\";\n" has failed [%basis
 GTOName ="system.bas";
  != %basis
 GTOName ="system.bas";
 GTOAuxName ="system.aux";
 ]
 *** 1 failure is detected in the test module "orca_test"

codecov · 2020-04-11T15:13:31Z

Codecov Report

Merging #413 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #413   +/-   ##
======================================
  Coverage    64.3%   64.3%           
======================================
  Files         281     281           
  Lines       23663   23663           
======================================
  Hits        15233   15233           
  Misses       8430    8430

Flag	Coverage Δ
#gcc	`64.3% <0.0%> (ø)`

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 8eecbfd...8eecbfd. Read the comment docs.

junghans · 2020-04-11T16:47:29Z

@JensWehner please review! Do we need tests?

JensWehner

I have two major concerns with this PR still

a) I do not get why we update the defaults with user options?
b) I am not sure, if it is a good idea to default everything.

felipeZ · 2020-04-14T08:03:00Z

I have two major concerns with this PR still

a) I do not get why we update the defaults with user options?

I think that updating the defaults with the user input is the normal behavior that I would expect of a general simulation package. As an author of a simulation package, you don't want all your users to enter every single variable for configuring all the details of your simulation neither force them to make a copy of the whole options file and manually update the sections that the user are interested in. Instead, most simulation packages allow the user to provide minimal input that is subsequently merged with the whole set of configuration variables.

b) I am not sure, if it is a good idea to default everything.

Put yourself on the user's shoes, you would like to try this amazing XTP package to do a DFT-GWBSE simulation, if instead of having a very simple input interface (that can be modified for expert users) you force your users to enter every single detail of the simulation, the potential users would run away ;)
Also, we want to interface with Python using ASE. The philosophy of those wrappers is "plug and play", meaning that a minimal configuration is required to make a given package to run which is very useful when doing scripting.

JensWehner · 2020-04-14T12:51:05Z

Okay I agree with a).

Concerning b) I agree that most things can be defaulted, but inputfilenames should not be defaulted in my opinion.

felipeZ · 2020-04-14T13:03:18Z

Okay I agree with a).

How do you envision the input/defaults handling?

Concerning b) I agree that most things can be defaulted, but inputfilenames should not be defaulted in my opinion.

How shall we then handle the input filenames?

JensWehner · 2020-04-14T13:05:53Z

I thought we do it the same way as before. I agree that 90% of all options have a sensible default, but I do not think that the coordinates file, or the output file have that.

JensWehner · 2020-04-14T13:07:24Z

Apart from eanalyze and ianalzye I cannot see how you would default all options of a calculator.

junghans · 2020-04-14T22:19:18Z

@JensWehner please review.

JensWehner

I still do not think defaulting all options is a good idea, but the PR has a lot more good than bad things in it.

baumeier added the work in progress label Mar 31, 2020

felipeZ and others added 22 commits April 2, 2020 11:26

removed mo and log file options from gwbse engine #414

fbd7dc1

Merge pull request #415 from votca/felipe-defaults-xml

8c4bb9e

removed mo and log file options from gwbse engine

probably just cleaning

5478020

Merge branch 'defaults-xml' of https://github.com/votca/xtp into defa…

ae87272

…ults-xml

allow optional gwbse and dftpackage

069520d

make xtp_tools option file optional #319

7f82bab

removed redundant code

e23f4a2

update year

22c5707

Merge pull request #418 from votca/felipe-defaults-xml

55cfcb1

Felipe defaults xml

Merge branch 'master' into felipe-defaults-xml

6c25caf

make file optinal for aptdft #319

9f56cfc

Merge branch 'defaults-xml' of https://github.com/votca/xtp into defa…

97d7058

…ults-xml

fixed test orca

efa5136

make file optinal for gencube #319

ad7c3df

make file optinal for spectrum #319

8759039

added defaults

38b89d4

make file optional for log2mps #319

a9ba59e

#319 added default for partial charge

4bcf5fb

make options files optional for molpol calculator #319

c3d967a

added defaults for densityanalysis #319

f8193b4

rename Initialize arg to user_input #319

fdab7d8

allow optional file for coupling #319

5500b67

make format

191ecb6

junghans removed the work in progress label Apr 9, 2020

junghans mentioned this pull request Apr 9, 2020

Xml new default handling votca/votca#263

Merged

junghans changed the title ~~[WIP] Reworked Default/Optional XML~~ Reworked Default/Optional XML Apr 9, 2020

junghans reviewed Apr 9, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

felipeZ and others added 3 commits April 10, 2020 08:15

fixed orca test

80168d3

format orca test

a6ed8c1

Update CHANGELOG.md

d3e52c5

junghans mentioned this pull request Apr 10, 2020

fix build with gcc-10 #424

Merged

junghans requested a review from JensWehner April 13, 2020 14:09

JensWehner suggested changes Apr 13, 2020

View reviewed changes

felipeZ added 2 commits April 14, 2020 16:26

use an explicit Property instead of _options private variable

33049ad

make calculator Initialize input const

5e92833

junghans mentioned this pull request Apr 14, 2020

update tools and xtp for new default handling votca/votca#265

Merged

make format

8eecbfd

junghans requested a review from JensWehner April 14, 2020 22:19

JensWehner approved these changes Apr 14, 2020

View reviewed changes

junghans approved these changes Apr 15, 2020

View reviewed changes

junghans merged commit c8fc9bd into master Apr 15, 2020

junghans deleted the defaults-xml branch April 15, 2020 01:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworked Default/Optional XML #413

Reworked Default/Optional XML #413

baumeier commented Mar 31, 2020 •

edited by junghans

Loading

ghost commented Mar 31, 2020 •

edited by ghost

Loading

JensWehner commented Apr 1, 2020

baumeier commented Apr 1, 2020

JensWehner commented Apr 2, 2020

baumeier commented Apr 2, 2020 •

edited

Loading

junghans commented Apr 9, 2020

junghans commented Apr 9, 2020

codecov bot commented Apr 11, 2020 •

edited

Loading

junghans commented Apr 11, 2020

JensWehner left a comment

felipeZ commented Apr 14, 2020 •

edited

Loading

JensWehner commented Apr 14, 2020

felipeZ commented Apr 14, 2020

JensWehner commented Apr 14, 2020

JensWehner commented Apr 14, 2020

junghans commented Apr 14, 2020

JensWehner left a comment

Reworked Default/Optional XML #413

Reworked Default/Optional XML #413

Conversation

baumeier commented Mar 31, 2020 • edited by junghans Loading

ghost commented Mar 31, 2020 • edited by ghost Loading

DeepCode failed to analyze this pull request

JensWehner commented Apr 1, 2020

baumeier commented Apr 1, 2020

JensWehner commented Apr 2, 2020

baumeier commented Apr 2, 2020 • edited Loading

junghans commented Apr 9, 2020

junghans commented Apr 9, 2020

codecov bot commented Apr 11, 2020 • edited Loading

Codecov Report

junghans commented Apr 11, 2020

JensWehner left a comment

Choose a reason for hiding this comment

felipeZ commented Apr 14, 2020 • edited Loading

JensWehner commented Apr 14, 2020

felipeZ commented Apr 14, 2020

JensWehner commented Apr 14, 2020

JensWehner commented Apr 14, 2020

junghans commented Apr 14, 2020

JensWehner left a comment

Choose a reason for hiding this comment

baumeier commented Mar 31, 2020 •

edited by junghans

Loading

ghost commented Mar 31, 2020 •

edited by ghost

Loading

baumeier commented Apr 2, 2020 •

edited

Loading

codecov bot commented Apr 11, 2020 •

edited

Loading

felipeZ commented Apr 14, 2020 •

edited

Loading