felipeZ commented 3 years ago

Changes

Fix #606
Remove the singleton pattern for all factories
Create Python interface to Calculators

codecov[bot] commented 3 years ago

Codecov Report

Merging #673 (5f4b346) into master (4492e03) will increase coverage by 2.9%. The diff coverage is 80.2%.

@@           Coverage Diff            @@
##           master    #673     +/-   ##
========================================
+ Coverage    51.6%   54.6%   +2.9%     
========================================
  Files         292     295      +3     
  Lines       27029   27076     +47     
========================================
+ Hits        13969   14787    +818     
+ Misses      13060   12289    -771

Impacted Files	Coverage Δ
include/votca/xtp/jobcalculatorfactory.h	`0.0% <0.0%> (-100.0%)`	:arrow_down:
include/votca/xtp/qmregion.h	`0.0% <0.0%> (ø)`
src/libxtp/factories/jobcalculatorfactory.cc	`0.0% <0.0%> (-100.0%)`	:arrow_down:
src/libxtp/jobcalculators/eqm.cc	`0.0% <0.0%> (ø)`
src/libxtp/jobcalculators/iqm.cc	`0.0% <0.0%> (ø)`
src/libxtp/qmregion.cc	`0.0% <0.0%> (ø)`
src/libxtp/tools/coupling.h	`0.0% <0.0%> (ø)`
src/libxtp/tools/log2mps.h	`0.0% <0.0%> (ø)`
src/libxtp/tools/molpol.cc	`0.0% <0.0%> (ø)`
src/tools/xtp_parallel.cc	`39.2% <33.3%> (ø)`
... and 63 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 4492e03...5f4b346. Read the comment docs.

felipeZ commented 3 years ago

@votca-bot format

felipeZ commented 3 years ago

@junghans what is the best way to install the resulting binding libraries for Python? The Potential Python users needs to include that library in their PythonPath

junghans commented 3 years ago

ok, will look at it soon.

junghans commented 3 years ago

@votca-bot format

junghans commented 3 years ago

ok, it builds now, but we need some tests.

felipeZ commented 3 years ago

@JensWehner @junghans @baumeier I have a problem trying to capture the standard output of the binds. when the library calls a class, for instance DFTGWBSE, all the output in the C++ side is redirect to standard output. Now, I cannot capture that because it is outside Python. If I am invoking the binaries, like xtp_tools I can always redirect the output to a file but the problem is that inside Python, the runtime is not aware that the invoke library is printing something.

If you run the example you will see what I mean. Something like:

>>> ./examples.py
calling dftgwbse
 Using 12 threads
... ... Reading structure from files_examples/methane.xyz
... ... 2021-2-3 14:26:57 Using 12 threads
... ... 2021-2-3 14:26:57 Using MKL overload for Eigen 
... ...  Molecule Coordinates [A] 
... ...   C   +30.3000 +1.2728 +5.5213
... ...   H   +31.2389 +1.4779 +6.0335
... ...   H   +30.4900 +0.6540 +4.6465
... ...   H   +29.8465 +2.2131 +5.2108
... ...   H   +29.6246 +0.7522 +6.1979
... ... 2021-2-3 14:26:57 Loaded DFT Basis Set 3-21G with 17 functions
... ... 2021-2-3 14:26:57 Loaded AUX Basis Set aux-def2-tzvp with 136 functions
... ... 2021-2-3 14:26:57 Total number of electrons: 10

But inside the example I am capturing the standard output. Can we redirect the log to a file?

JensWehner commented 3 years ago

I do not understand fully.

baumeier commented 3 years ago

@JensWehner @felipeZ I think the problem is that the code is logging to screen and that is not really what you want with the PyBind.

There used to be some sort of "silent mode" for the logger (redirect), for instance in qmmm I think, or also for the internal mode for geometry optimization, when you maybe didn't want the whole output of the 6N numerical gradient calculations messing out the console. I am not sure if this exists any more.

felipeZ commented 3 years ago

@JensWehner the problem that I have is indeed the one described by @baumeier. I want to either redirect the logger to a file or silence it

junghans commented 3 years ago

Should there be at least an __init__.py that includes everything?

baumeier commented 3 years ago

@felipeZ in gwbseengine.cc there is


  // Logger redirection
  _redirect_logger = options.ifExistsReturnElseReturnDefault<bool>(
      ".redirect_logger", _redirect_logger);
  _logger_file = "gwbse.log";
``

felipeZ commented 3 years ago

@felipeZ in gwbseengine.cc there is
  // Logger redirection
  _redirect_logger = options.ifExistsReturnElseReturnDefault<bool>(
      ".redirect_logger", _redirect_logger);
  _logger_file = "gwbse.log";
``
The problem is that I am not wrapping the GWBSEEngine class but the whole QMTool Factory and I need to setup the logger at that level.

junghans commented 3 years ago

Would using boost log help there?

JensWehner commented 3 years ago

Concerning the logging, I would accept the current behaviour for now and then do the logging in a second PR because it will be a lot of work.

So the current Logger class can be redirected to a file or any other stream. We still have a lot of cout commands which you cannot redirect easily. For that part I have no idea.

Can you elaborate on your idea how to redirect it?

felipeZ commented 3 years ago

@junghans @JensWehner @baumeier @rubengerritsen For the sake of brevity, I am not doing any further development and the logging can go in another PR. Notice that the Python/C++ interfaces are minimal functions that invoke either the XTP-Calculators or XTP-Tools. Further development can include exposing the classes themselves and/or having more sophisticated Python function to invoke the C++ functionality. The main goal of this PR is to show what is possible to do with Pybind11 and decided where to go from here.

felipeZ commented 3 years ago

a) Really great work and interesting b) thanks for refactoring all the factories c) I do not understand the pybind xtp_tools d) I am not sure about the examples folder and I am afraid that this will break all the time as do the tutorials. So at least for the xtp repository I only want stuff that is covered by tests.

Thank you for all the nice effort.

Thanks for the feedback! About the last point, I was thinking on replacing all the calls to the binaries in the xtp-tutorials by the Python calls to the xtp-binds in that way we show the use of xtp-binds and simultaneously check that everything is working. What do you think @junghans @baumeier?

JensWehner commented 3 years ago

That is a sensible plan, but it then makes pybind11 a hard dependency. It seems So I am all for it.

Can we discuss the examples part again?

felipeZ commented 3 years ago

@JensWehner about the examples, cmake is generating a pyxtp Python package and the examples are inside in such a way that the examples can be accessed with Python like:

from pyxtp.examples import run_examples

JensWehner commented 3 years ago

yes, but I am afraid, that they always will be forgotten to be updated and the binary file I am not happy about either. :(

So a) if we have to keep it, we have to test the hell out of it b) I would prefer a more clever solution, but I do not have one atm.

felipeZ commented 3 years ago

I agree with you @JensWehner about the examples. So, I am going to remove them and instead once the xtp_binds are available I will use them instead of the system call in xtp-tutorial

junghans commented 3 years ago

Couldn't we just run the example as part of a test?

junghans commented 3 years ago

@felipeZ see here https://github.com/votca/csg/blob/master/src/tools/CMakeLists.txt#L43-LL47

you will need to do something like:

add_test(NAME integration_Run_pyxtp COMMAND ${Python_EXECTUABLE} -c "import pyxtp")
set_tests_properties(integration_Run_pyxtp PROPERTIES LABELS "csg;tools;votca;xtp;integration")
add_test(NAME integration_Compare_pyxtp_output COMMAND $<TARGET_FILE:VOTCA::votca_compare>  -f1 file1 -f2 ${REFPATH}/file2)
set_tests_properties(integration_Compare_pyxtp_output PROPERTIES DEPENDS integration_Run_pyxtp)
set_tests_properties(integration_Compare_pyxtp_output PROPERTIES LABELS "csg;tools;xtp;votca;integration")

felipeZ commented 3 years ago

@felipeZ see here https://github.com/votca/csg/blob/master/src/tools/CMakeLists.txt#L43-LL47

you will need to do something like:

add_test(NAME integration_Run_pyxtp COMMAND ${Python_EXECTUABLE} -c "import pyxtp")
set_tests_properties(integration_Run_pyxtp PROPERTIES LABELS "csg;tools;votca;xtp;integration")
add_test(NAME integration_Compare_pyxtp_output COMMAND $<TARGET_FILE:VOTCA::votca_compare>  -f1 file1 -f2 ${REFPATH}/file2)
set_tests_properties(integration_Compare_pyxtp_output PROPERTIES DEPENDS integration_Run_pyxtp)
set_tests_properties(integration_Compare_pyxtp_output PROPERTIES LABELS "csg;tools;xtp;votca;integration")

I have added the following in the CMakeLists.txt of tests:

  add_test(NAME integration_test_xtp_binds COMMAND ${Python_EXECUTABLE} -c "from pyxtp.xtp_binds import *")
  set_tests_properties(integration_test_xtp_binds PROPERTIES LABELS "csg;tools;votca;xtp;integration")
  add_test(NAME integration_Compare_pyxtp_output COMMAND $<TARGET_FILE:VOTCA::votca_compare>  -f1 file1 -f2 ${REFPATH}/file2)
  set_tests_properties(integration_Compare_pyxtp_output PROPERTIES DEPENDS integration_test_xtp_binds)
  set_tests_properties(integration_Compare_pyxtp_output PROPERTIES LABELS "csg;tools;xtp;votca;integration")

junghans commented 3 years ago

@felipeZ

85/86 Test #275: integration_test_xtp_binds ................***Failed    0.25 sec
Calling mapchecker
Traceback (most recent call last):
  File "/__w/xtp/xtp/votca/xtp/src/pyxtp/examples.py", line 69, in <module>
    run_examples()
  File "/__w/xtp/xtp/votca/xtp/src/pyxtp/examples.py", line 64, in run_examples
    run_mapchecker(nThreads, path_examples)
  File "/__w/xtp/xtp/votca/xtp/src/pyxtp/examples.py", line 37, in run_mapchecker
    xtp_binds.call_calculator("mapchecker", dict_inp)
RuntimeError: Error on open xml file: /__w/xtp/xtp/builddir/xtp/src/pyxtp/files_examples/mapchecker.xml: iostream error

felipeZ commented 3 years ago

@junghans do you have an idea about where the following error is coming from?

 ... ... 2021-2-9 12:39:34 Loaded DFT Basis Set 3-21G with 17 functions
... ... 2021-2-9 12:39:34 Loaded AUX Basis Set aux-def2-tzvp with 136 functions
... ... 2021-2-9 12:39:34 Total number of electrons: 10
 ... ... 2021-2-9 12:39:34 Filled DFT Overlap matrix.INTEL MKL ERROR: /opt/intel/mkl/lib/intel64/libmkl_avx512.so: undefined symbol: mkl_sparse_optimize_bsr_trsm_i8.
Intel MKL FATAL ERROR: Cannot load libmkl_avx512.so or libmkl_def.so.

JensWehner commented 3 years ago

So the avxlibrary is normally only linked with vectorisation enabled could be a difference between numpy and C++ compiler flags?

felipeZ commented 3 years ago

So the avxlibrary is normally only linked with vectorisation enabled could be a difference between numpy and C++ compiler flags?

But we are not using Numpy at all

JensWehner commented 3 years ago

Then I have no idea. :(

junghans commented 3 years ago

Reading https://groups.google.com/g/kaldi-help/c/m3nyQke0HS0/m/4fj8gkSWAgAJ, we need to link MKL::RT instead MKL::Shared to overcome this issue. Fix in votca/tools#349.

JensWehner commented 3 years ago

@votca-bot format

junghans commented 3 years ago

Replaced by https://github.com/votca/votca/pull/842

votca / xtp

Pybind11 #673

Changes

Codecov Report