JOSS Review: Installation

[lheagy]

Originally from @jochym in openjournals/joss-reviews#440

• The list of dependencies is huge. It may be fully justified but from the docs (just two functions in the API) it seems that the dependency management is too careless. Furthermore it depends on packages from four (!) different conda channels. This install is bound to have problems quite soon due to the version drift and incompatibilities - see below.
• The package is written in the heading into obsolescence python 2.7 (EOL in 2020). The process of adoption of python 3 is speeding up quite a bit recently. Multiple new developments in data science already depend on python 3. Including technologies used by the authors - parts of jupyter are already python 3 only. This cannot be a reason for rejection but I would encourage authors to port to python 3 soon - it is easier than expected.
• Such complicated install forces the package into separate virtual environment and makes it difficult or impossible to integrate the package into larger production environment. E.g. it would be impossible to integrate the package into standard jupyterhub environment without major work and inconveniences. This substantially decreases the utility of the software.
• Non-specified dependency on pyversion makes 5 tests to fail. Furthermore this dependency cannot be installed from conda, it's last release on pypi was in 2015 and does not support python 3. Essentially package cannot be fully tested using described install procedure.

Items to be addressed

• [ ] dependencies: is the large dependency list necessary? If so, could this be explained in the documentation, otherwise, removing dependencies that are not necessary will simplify installation and maintenance
• [ ] is there a plan to upgrade to python 3? This does not need to be completed prior to an accepted publication, but explaining if this is in the roadmap in the documentation would be helpful
• [ ] can the installation process be simplified? if not, please provide some explanation in the documentation
• [ ] pyversion dependency and failing tests

[yxqd]

Dependencies

This deserves a bit of explanation. The fact that the current dependency is long is probably mostly due to it now depends on mantid, which is a huge project and depends on a lot of packages. The core functionality of the multiphonon package (converting from I(Q,E) spectrum to DOS spectrum) does not require Mantid, as you can see in this simple examle. The Mantid package is used solely for reducing the raw data from the inelastic neutron scattering experiment to I(Q,E) spectrum. So we could remove the mantid dependency to make this package minimal. The reason we had Mantid in the dependency list was for practical usage of the multiphonon package. This software, deployed at the SNS jupyter hub environment, is for users of the direct geometry inelastic neutron scattering instruments, and the Mantid software is the standard solution for reducing the raw data at these instruments. It is almost inevitable for users of this package to be using mantid at some stage, at least for now. However, I agree that this binding of the multiphonon package and the mantid software can happen in some other way. So I plan to

• [x] remove Mantid from the dependency list, and
• [x] instruct users that he most likely needs to install mantid in order to work with their experimental data

BTW, the current direct dependency list of multiphonon is:

- numpy
- histogram
- scipy
- matplotlib
- pytest
- mantid-framework
- ipywe

Here numpy scipy matplotlib pytest are all quite standard dependencies for data analysis packages; histogram and ipywe are tiny compared to mantid.

Python 3

Since the most likely workflow for users involves using Mantid, which is now in python 2, we did not try to implement it in python 3. But making it python 3 ready is in our plan. I will create an issue to track that.

Simplify installation Removing mantid as a dependency will simplify installation.

pyversion dependency I just checked the conda environments I manage and did not see "pyversion" package installed in them. I also tried fresh installations of multiphonon on a ubuntu 16.04 using the latest miniconda2/miniconda3, and did not see pyversion either. @jochym, could you please let us know how to reproduce this problem? Thanks.

[yxqd]

@jochym @lheagy please comment if this looks reasonable to you. Thanks!

[jochym]

pyversion.

I have created the install in fairly standard way:

• conda create -n review -c conda-forge python=2.7
• source activate review
• conda install -c ....... multiphonon

Then after cloning of the distro I have tried to run the tests as described in the docs. Five tests failed due to the pyversion dependency. This was run on my debian testing (buster) machine. I can try the same in the clean debian stretch virtual machine but probably in few days.

dependencies

I understand many users of multiphonon will also use mantid but this is not a good reason to make it a dependency if it really does not depend on it for its functionality. The dependency list should be as short as possible in my opinion. Let the users decide what they need. You can always create simple meta-package (or even better conda environment) for your local setup which just depends on popular packages.

python 3

As I stated in my review this is not a show-stopper, but I predict that the problems with increasing obsolescence of py2 packages will soon be substantial.

Installation

It would help to either make a copy of packages from other channels or, better still, package missing parts for conda-forge. Again, I do not consider this a show-stopper.

[yxqd]

Thanks, @jochym. I agree a meta-package is a great idea. And being python-3 compatible is our next goal.

For installation, we removed Mantid from dependency list and updated instructions. We also improved mantid conda package dependency list just in case you also want to test mantid-related functionalities in multiphonon. Please give it another try. See readme in examples

[jochym]

Following will be my report on installation/testing. I will amend the text as I go along the steps

1. The basic installation on the headless debian system worked fine but failed at the first example due to the lack of X11 display. I think the simple test should not require graphics if the output is graphical it should be saved to the file - this way the package can be tested in the headless environment. Also the install is still quite substantial - but I guess this cannot be easily reduced further. However, out of my curiosity: why what seems to be a computational package has so many dependencies? I would like to understand that, and be grateful if authors would comment on that point. For the install recording see: https://asciinema.org/a/Enhu0liwu5hXNwzNnT2x1lJrd
2. The example notebook required even more dependencies, as I already noted. To be fair, many of them will be already present on the typical data-science environment. On the other hand I would expect them to be present in the py3 variant and that means carrying out and maintaining separate large install just for multiphonon. This is sub-optimal, in my opinion, and limits usefulness of the package. The size of the fully configured env with just multiphonon dependencies is 2.4GB (!), and I do not expect this data to be shared with other environments due to the python version difference. Disk space is cheap these days but this is a substantial setup which will need maintaining - and admin/user time is not getting cheaper.
3. Running the notebook led to the following error (mantid-framework is installed):

   
   Converting sample data to powder I(Q,E)...
   * Checking Mantid ...
   ---------------------------------------------------------------------------
   RuntimeError                              Traceback (most recent call last)
   /home/user/miniconda3/envs/review/lib/python2.7/site-packages/ipywe/wizard.pyc in handle_next_button_click(self, s)
    37             return
    38         self.remove()
   ---> 39         self.nextStep()
    40 
    41     def validate(self):

/home/user/miniconda3/envs/review/lib/python2.7/site-packages/multiphonon/ui/getdos.pyc in nextStep(self) 202 workdir=context.workdir, 203 ) --> 204 for msg in r2i:print msg 205 context.iqe_h5, context.mtiqe_h5, context.Qaxis, context.Eaxis = msg 206 print "Done."

/home/user/miniconda3/envs/review/lib/python2.7/site-packages/multiphonon/getdos.pyc in reduce2iqe(sample_nxs, Emin, Emax, dE, Qmin, Qmax, dQ, mt_nxs, iqe_nxs, iqe_h5, workdir) 123 Qaxis = _normalize_axis_setting(Qmin, Qmax, dQ) 124 yield "Converting sample data to powder I(Q,E)..." --> 125 raw2iqe(sample_nxs, iqe_nxs, iqe_h5, Eaxis, Qaxis, type='sample') 126 if mt_nxs is not None: 127 _tomtpath = lambda p: os.path.join(

/home/user/miniconda3/envs/review/lib/python2.7/site-packages/multiphonon/getdos.pyc in raw2iqe(eventnxs, iqe_nxs, iqe_h5, Eaxis, Qaxis, type) 181 os.remove(iqe_nxs); os.remove(iqe_h5) 182 # --> 183 from .redutils import reduce, extract_iqe 184 Emin, Emax, dE = Eaxis 185 Emin-=dE/2; Emax-=dE/2 # mantid algo use bin boundaries

/home/user/miniconda3/envs/review/lib/python2.7/site-packages/multiphonon/redutils.py in () 26 return 27 if not mantid_checked: ---> 28 _checkMantid() 29 30

/home/user/miniconda3/envs/review/lib/python2.7/site-packages/multiphonon/redutils.py in _checkMantid() 20 sp.call(shlex.split("python -c 'import mantid'"), stdout=sp.PIPE, stderr=sp.PIPE) # sometimes mantid import for the first time may fail 21 if sp.call(shlex.split("python -c 'import mantid'")): ---> 22 raise RuntimeError("Please install mantid") 23 global mantid_checked 24 mantid_checked = True

RuntimeError: Please install mantid

4. Tests: 
  - The "no mantid, no ipwe" test run to completion, however with the two warnings:
```warnings summary:
ms_TestCase.py::TestCase::test1
  /home/user/miniconda3/envs/review/lib/python2.7/site-packages/histogram/hdf/Loader.py:129: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
    if 'storage' in list(dataGroup): # this uses the 'storage' convention

backward/singlephonon_sqe2dos_TestCase.py::TestCase::test1c
  /home/user/multiphonon/multiphonon/backward/singlephonon_sqe2dos.py:158: UserWarning: Scaling factor to combine DOSes calculated is not stable: 22.1354562834 (using continuous criteria) vs 17.7865062045 (using area criteria)
  You may want to consider adjusting the parameters such as E range (Emax more specifically)
    scale1, scale2)

The last warning seems troubling - is that expected? In the test case?

• The full test set failed with 5 failing tests and RuntimeError: Please install mantid message. As I stated above mantid-framework is installed.

All above tests were performed in the clean, fresh install of debian 9.3 on the headless virtual machine. The tests on workstation install will follow in the next post.

[jochym]

Tests with desktop system:

1. Install went the same - no problems this time
2. All tests executed, eventually.
3. The tests seem to be rather memory-hungry. I was unable to complete all of them on the system with 4GB of RAM. They finally completed when I increased memory to 8GB but 5GB would probably be enough. It is rather large. Is this an inherent property of the code or could authors do something about it? If nothing could be done the docs should clearly state this requirement - for that type of calculation I did not expect such requirements.
4. The example notebook run to completion and produced expected results.
5. I am not too fond of "wizards" used in the notebook. They are not very clear and break a natural workflow of the document. I would rather have clearly commented dictionaries with parameters and functions you can just run and wait for the result. Note that this type of the interface makes "run all cells" command useless. The same goes for expected in the future notebook import ability of the jupyter system. If the authors want to build an interactive dashboard/application they should do that. I my opinion it is better when the notebook keeps the document metafor. The wizards bring unnecessary "moving parts" to the example.
6. The "instability" warnings kept showing up in the test results. I think authors should address this concern.
7. The full system (with jupyter interface) cannot be used on the headless server - it seems to me. I suspect authors cannot do anything about it, as I think this limitation comes from mantid component. At the same time this is not a real limitation since the process is not actually using any X11 graphics - it just expects to have one active. This limits the usability of the system. E.g. I could not use it on my production system as this is purely headless, networked jupyterhub install on the cloud service. I would like to have authors address/comment this point as well.

[jochym]

@yxqd @lheagy : Final remarks (install)

The basic problems are resolved. The summary of remaining install issues:

1. Problems on headless system with non-helpful error messages. Either solve the problem or explain it in the docs. It will be very confusing for inexperienced user.
2. Memory requirements should be explained/documented or reduced
3. Address instability warnings in the tests
4. On my standard system - my work laptop the mantid/ipywe tests still fail with segmentation faults and mantid not found errors:

   ********* SEGMENTATION FAULT *********
   /home/jochym/miniconda/envs/review/lib/python2.7/site-packages/mantid/api/../../../../libMantidAPI.so(+0x162586) [0x7f22a6035586]
   /home/jochym/miniconda/envs/review/lib/python2.7/site-packages/mantid/api/../../../../libMantidAPI.so(+0x16275f) [0x7f22a603575f]
   /lib/x86_64-linux-gnu/libc.so.6(+0x33af0) [0x7f22c0296af0]
   [0xfdfe30]

   I cannot hold authors responsible for the single system problem, but this system works fine with multiple sets of conda packages and multiple environments - I am a havy python/jupyter user for solid state physics. This may suggest some issues with the mantid package interaction. One more reason for limiting number of outside dependencies.

5. For the same reason the notebook example did not work on my standard system
6. Please, at least comment on the issues noted in my two above posts.

[yxqd]

Thanks @jochym! I will try to add a new issue on making things working on a headless system (we did not expect this kind of usage but I agree this might come up in the future). Regarding memory requirements: the mantid reduction requires quite a bit of memory. I will try to add comments to documentation about this. It looks like mantid installation using conda is still causing problems on your standard system. Most of our current users are using the SNS analysis clusters (redhat) in which mantid conda installation does work. Fixing the mantid-conda installation is beyond the scope of this paper I think. And I agree leaving it out of the dependency list is the right call. Sorry I am not back to work yet and will add more details soon, most likely tomorrow.

[jochym]

Do not worry too much about my system - you are not responsible for such things. My system is not standard and some brakages may be expected. It is important that the soft work on standard released distros - and it does at least on last debian. Really important point is the headless systems. This is quite common use case (cloud services!) and I believe there is no real requirement for X11 in mantid. Maybe it can be just reconfigured to use different backend at runtime or something? It does not actually use X11 when it runs in the notebook.

[yxqd]

Thanks @jochym. Glad that the software works for standard debian linux distributions. We have only tested ubuntu/fedora/redhat before.

Regarding headless systems, it turns out the core functionality of multiphonon works. The example script failed at a headless system only because it generates a plot at the end for users to interact with. I just created #100 to address this.

The Mantid software was started about 10 years ago and at that time it was not really intended to be a python library. Instead, the focus was mantidplot, the GUI. Only very recently it is pushed for having a stand-alone python library. So I think the task of making sure mantid works for a variety of headless systems is a long battle for the future. For example, I tried installing mantid-framework conda package on a miniconda docker image (which derives from debian), and "import mantid" only works after "freeglut3-dev" is installed using apt-get. I hope you agree that this is out of scope for this paper review.

[yxqd]

About the last warning in the test: it is expected since we are still experimenting on this new feature for "stitching" together phonon DOSes measured at different neutron incident energies. Right now researchers more or less do the stitching manually and somewhat arbitrarily, and this software provides two routines for stitching. The warning is to alert the users that two methods of stitching were used but they disagree somewhat in the calculation of scaling factor for stitching. The disagreement is actually acceptable in this case.

[jochym]

1. Regarding mantid - this is fine. You are not submitting mantid for review. You are not responsible for its deficiencies. But they influence your software in quite cryptic way, and at the beginning of the learning curve. Thus I think that these problems should be at least mentioned (better still explained) in the installation docs.
2. This text about the warning should be included in the docs - it will help users gain confidence in the package.

[yxqd]

@jochym @lheagy We made the following improvements:

• Moved the majority of the installation and usage documentation from README.md to the documentation site http://sns-chops.github.io/multiphonon/ and expand them:
  • explained limitations of mantid-framework conda installation
  • added a section on system requirements
  • added a section on "stitching": http://sns-chops.github.io/multiphonon/usage.html#working-with-datasets-from-multiple-incident-energies
• Add new example notebook and script:
  • Example notebook without UI widgets
  • Example script for stitching

[lheagy]

Thanks @yxqd, @jochym, in your opinion, can we close this issue?

[jochym]

I'll check. Probably not today, but I will try ASAP.

[lheagy]

hi @jochym and @yxqd, just checking in. Any updates?

[yxqd]

Thanks @lheagy. @jochym could you please let us know your comments?

[jochym]

I like what you did with the docs and install description. I would include the actual command-by-command tutorial in the docs, but it is present in the repo and linked from the docs so this is only advice not a complaint. Please add that it will probably not work on the headless system without special config - it is better to not feed false hopes of the users. I like what you did with the examples as well. I would suggest considering producing mybinder (https://mybinder.org/) repo for the package. It is quite easy for the conda installable package, and it would help users to check if they have the install correctly working. But this is only a suggestion, and not a part of the review. I had to add import mantid to the notebook for it to work on my system. It took several minutes maybe warn the users to expect long run times and not panic? It will be better to add this even if on some systems I tested it was not required - it cannot hurt. And maybe change download part to not run each time if the data is already there ? Otherwise I think this issue can be closed after this small things are fixed.

Essentially this summarizes my opinion on https://github.com/sns-chops/multiphonon/issues/83 and https://github.com/sns-chops/multiphonon/issues/84 as well. Which means that these are my final remarks @lheagy.

[yxqd]

Thanks @jochym. I put in some minor fixes as you suggested and created tickets for some other minor points to work on later. I will close this ticket and #83, #84 now. Please let me know if there are any other problems.