ISISNeutronMuon / MDANSE

MDANSE: Molecular Dynamics Analysis for Neutron Scattering Experiments
https://www.isis.stfc.ac.uk/Pages/MDANSEproject.aspx
GNU General Public License v3.0
21 stars 5 forks source link

ENHANCEMENT: Use netcdf4 #67

Closed eurydice76 closed 2 years ago

eurydice76 commented 2 years ago

As agreed, MDANSE should be detached from ScientificPython and MMTK sooner or later (see protos branch for a putative repacement candidate). To do so, we should rely on netcdf4 library instead of Scientific.IO.NetCDF which use a C extension nearly impossible to port to python 3. The API is nearly the same than the one of Scientific but it has the advantage of being pip-installable on python 2 and 3.

eurydice76 commented 2 years ago

Unfortunately netCDF4 is not easily installable with python2. A basic pip install netCDF4 fails.

eurydice76 commented 2 years ago

I tried to install it from source but I gave up due to very strange errors at install process. However, a simple conda install netCDF4 does the job.

eurydice76 commented 2 years ago

Hi @RastislavTuranyi, I just pushed on branch #67 the change from Scientific.IO.NetCDF to netCDF4. Everything seems OK from the code point of view. However, I have two concerns. The first one is about the build. We have to add a dependency to netCDF4 package. This can be installed easily using conda. At the time, I wrote the complete build server scripts but I lost the hand on this and I do not know how it works now on github. Could you add this dependency on Windows, Linux and MacOS easily ? The second one is about failing tests. Here are the tests that are failing:

======================================================================
FAIL: test_discover (Test_discover.TestDISCOVER)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_discover.py", line 44, in test_discover
    self.assertTrue(compare("../../../Data/Jobs_reference_data/discover_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

======================================================================
FAIL: test_pdf (Test_pdf.TestPDF)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_pdf.py", line 55, in test_pdf
    self.assertTrue(compare("../../../Data/Jobs_reference_data/pdf_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

======================================================================
FAIL: test_castep (Test_castep.TestCASTEP)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_castep.py", line 43, in test_castep
    self.assertTrue(compare("../../../Data/Jobs_reference_data/castep_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

======================================================================
FAIL: test_short_header (Test_castep.TestCASTEP)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_castep.py", line 64, in test_short_header
    self.assertTrue(compare("../../../Data/Jobs_reference_data/castep_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

======================================================================
FAIL: test_dmol (Test_dmol.TestDMOL)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_dmol.py", line 44, in test_dmol
    self.assertTrue(compare("../../../Data/Jobs_reference_data/dmol_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

======================================================================
FAIL: test_rbt (Test_rbt.TestRBT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_rbt.py", line 48, in test_rbt
    self.assertTrue(compare("../../../Data/Jobs_reference_data/rbt_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

======================================================================
FAIL: test_dftb (Test_dftb.TestDFTB)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_dftb.py", line 44, in test_dftb
    self.assertTrue(compare("../../../Data/Jobs_reference_data/dftb_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

======================================================================
FAIL: test_forcite (Test_forcite.TestFORCITE)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pellegrini/git/MDANSE/Tests/FunctionalTests/Jobs/Test_forcite.py", line 44, in test_forcite
    self.assertTrue(compare("../../../Data/Jobs_reference_data/forcite_reference.nc", reference_data_path + "_mono" + ".nc"))
AssertionError: False is not true

----------------------------------------------------------------------

I suspect that these failures are due to the recent changes we did with the velocities stuff. Could you please investigate on this ?

RastislavTuranyi commented 2 years ago

If it can be installed with conda, there should be no issue with installing it in the CI/CD since conda is pre-installed on GitHub runners.

About the failing tests, almost all of them are due to the recent changes, as documented in #66. The only test that you have shown is failing but should not be is RBT. There have been no changes to RigidBodyTrajectory and I am not aware of this test failing at any point.

eurydice76 commented 2 years ago

OK, good so we can proceed to Review/PR. By the way, the failing test for rbt is normal, while refactoring the code, I found al old bug in the analysis. The RBT trajectory has to be recomputed. I will take that in charge.