search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
700 stars 36 forks source link

[REVIEW]: DMT-core: A Python Toolkit for Semiconductor Device Engineers #4298

Closed editorialbot closed 2 years ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@miesli<!--end-author-handle-- (Mario Krattenmacher) Repository: https://gitlab.com/dmt-development/dmt-core Branch with paper.md (empty if default branch): Version: 1.6.2 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @dilawar, @phoebe-p Archive: 10.5281/zenodo.6685185

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/f9829bb4e4c10b85144b22f888756045"><img src="https://joss.theoj.org/papers/f9829bb4e4c10b85144b22f888756045/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/f9829bb4e4c10b85144b22f888756045/status.svg)](https://joss.theoj.org/papers/f9829bb4e4c10b85144b22f888756045)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@dilawar & @phoebe-p, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @lucydot know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @dilawar

📝 Checklist for @phoebe-p

editorialbot commented 2 years ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.48 s (509.3 files/s, 208636.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
TeX                             39            145            269          49619
Python                          85           4348           7261          18441
CSS                              5             75            108           8810
LESS                            48            838            961           4524
JSON                             2              0              0           1952
reStructuredText                52            473            644            511
HTML                             3             12              0            276
SparForte                        1              3              0            189
Markdown                         2             53              0            152
YAML                             2             10             21            123
JavaScript                       1              5             12             49
DOS Batch                        1              8              1             27
TOML                             1              0              0             10
make                             1              4              6             10
Bourne Shell                     1              1              0              6
-------------------------------------------------------------------------------
SUM:                           244           5975           9283          84699
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 2 years ago

Wordcount for paper.md is 1239

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/MS.2010.152 is OK
- 10.2172/1826862 is OK
- 10.25080/majora-92bf1922-00a is OK
- 10.1109/MMM.2021.3117139 is OK
- 10.1109/JEDS.2020.3023165 is OK
- 10.1109/TED.2021.3051552 is OK
- 10.1109/bcicts50416.2021.9682487 is OK
- 10.1109/bcicts50416.2021.9682485 is OK
- 10.1109/bcicts50416.2021.9682473 is OK
- 10.1109/JEDS.2020.3023165 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

dilawar commented 2 years ago

Review checklist for @dilawar

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

phoebe-p commented 2 years ago

Review checklist for @phoebe-p

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

dilawar commented 2 years ago

@phoebe-p I edited your checklist by mistake 😞. I think I have reverted the changes I made.

phoebe-p commented 2 years ago

@miesli just wanted to report I had a small issue when installing DMT-core, similar to these issues: link 1, link 2, getting an error which boils down to:

.. ERROR:: Could not find a local HDF5 installation.

This is likely because I am using an Apple machine with an ARM/M1/Apple silicon chip, which causes all sorts of problems like this... I won't open an issue about this in the DMT-core repository because it is not actually an issue with your software (and in any case you say in the documentation that it is not tested on Mac), but with PyTables/HDF5. Going off the solutions discussed in links above, I was able to solve the problem relatively easily by installing HDF5 using homebrew:

brew install hdf5

and then adding the following line to my ~/.zshrc file (for setting environment variables in the command line environment on new versions of Mac, replacing .bashrc):

export HDF5_DIR="/opt/homebrew/opt/hdf5"

After this, the installation proceeded without errors and everything seems ok.

miesli commented 2 years ago

@phoebe-p thank you for the report and also for solving this issue. I will add your explanations and also your links to the DMT installation help.

Good to know that you are testing it on mac. I'm quite curious if there are more issues, but I hope that there are no more of them....

dilawar commented 2 years ago

@miesli I am trying to install it on a Debian-11 machine. python3.8 -m pip install DMT-core was successful but I ran into the following problem.

>>> from DMT.core import specifiers
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/dilawar/Work/GITHUB/JOSS/DMT-core/DMT/core/__init__.py", line 26, in <module>
    __version__ = semver.VersionInfo(major=1, minor=4, patch=0, prerelease="rc.1")
AttributeError: module 'semver' has no attribute 'VersionInfo'
>>>

semver.VersionInfo was deprecated and now has been removed (https://python-semver.readthedocs.io/en/latest/migration/migratetosemver3.html?highlight=VersionInfo#use-version-instead-of-versioninfo). Looks like that pip pulled in semver3 on my machine and it broke DMT-core. We can either use https://python-semver.readthedocs.io/en/latest/migration/migratetosemver3.html?highlight=VersionInfo#use-version-instead-of-versioninfo or pin the semver to a specific version?

miesli commented 2 years ago

@dilawar good to know. I did not check for the deprecation in the semver package.

I would try to resolve this issue for both versions with the code:


try:
    from semver.version import Version as VersionInfo
except ImportError:
    from semver import VersionInfo

__version__ = VersionInfo(major=1, minor=5, patch=0, prerelease="rc.2")

I will push this release candidate to pip. Then you should be able to use pip to install DMT-core successfully.

Thanks for tracing the error down.

phoebe-p commented 2 years ago

Linking GitLab issue regarding installation on MacOS here to keep track of things.

phoebe-p commented 2 years ago

@miesli some minor typos in the paper:

In the bullet list of 'Related publications', I wonder if there is some way in Markdown to remove the brackets around the references (@lucydot)? It looks a bit odd like this. Not a big issue though.

dilawar commented 2 years ago

@miesli Heads up about numpy and Numba. Numba (https://pypi.org/project/numba/0.55.1/) expects numpy to be numpy<1.22,>1.18. You need to pin numpy's version as well.

PS: I had good experience managing dependencies with https://python-poetry.org/

miesli commented 2 years ago

@phoebe-p I fixed the typos in the paper. The brackets around the related publications are indicating references. We may be able to reformulate and put the cite at the end of an explaining sentence for each bullet. I just checked other joss papers, but I did not find one where they have a comparable situation (multiple similar applications). So I'm not quite sure, how to improve this section....

@dilawar Thanks for checking the required packages. Actually, numba is a historic relic inside the DMT-core (we have other internal packages, where we use it). So I removed numba and at the same time cleaned setup.py.

I introduced extra_requires and made some install configurations, which hopefully better describe the versatility we tried to implement. The optional packages are:


EXTRAS_REQUIRE = {
    "HDF5": ["tables"],
    "pyqtgraph": ["pyqtgraph"],
    "matplotlib": ["matplotlib"],
    "pyside2": ["PySide2"],
    "pyqt5": ["pyqt5"],
    "smithplot": ["matplotlib", "pysmithplot@git+https://github.com/miesli/pySmithPlot"],
    "develop": ["pylint", "black"],
    "latex": ["pylatex", "pylatexenc"],
    "remote": ["paramiko", "scp"],
}
EXTRAS_REQUIRE["full"] = list(set(chain(*EXTRAS_REQUIRE.values())))
EXTRAS_REQUIRE["full"].remove("PyQt5")  # not always needed

So in most cases you would do

pip install DMT-core[full]

to use all features.

I had a look at poetry, but I did not start using it. Checking all packages for the lowest possible version is not feasible for me at the moment :(. So currently it requires a new virtualenv for DMT with all packages up to date...

dilawar commented 2 years ago

@miesli I checkout out the lastest main (1e165b9c5f91deed4646c5cfc9e4f563b949c8ca) and build it successfully after commenting out numba in setup.py file. But ran into errors during the pytest test/ step with python3.10.

______________ ERROR collecting test/test_verilogae/test_get_param_list.py _______________
ImportError while importing test module '/home/dilawar/Work/GITHUB/FOSS/DMT-core/test/test_verilogae/test_get_param_list.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/test_verilogae/test_get_param_list.py:5: in <module>
    from DMT.core import MCard
DMT/core/__init__.py:47: in <module>
    from pint import UnitRegistry
/home/dilawar/.local/lib/python3.10/site-packages/Pint-0.19.1-py3.10.egg/pint/__init__.py:30: in <module>
    ???
/home/dilawar/.local/lib/python3.10/site-packages/Pint-0.19.1-py3.10.egg/pint/registry.py:69: in <module>
    ???
/home/dilawar/.local/lib/python3.10/site-packages/Pint-0.19.1-py3.10.egg/pint/parser.py:21: in <module>
    ???
E   ModuleNotFoundError: No module named 'pint._vendor'
================================ short test summary info =======

Looks like an issue with the pint library and is specific to Python3.10. Because I could not reproduce it with python3.8 on a Debian box, I did not open any issue on the dmt-core repo. I did not try python3.9.

I opened an issue on the pint repo https://github.com/hgrecco/pint/issues/1511.

dilawar commented 2 years ago

Linking the issue that summaries issues reported by a linter https://gitlab.com/dmt-development/dmt-core/-/issues/4

miesli commented 2 years ago

@miesli I checkout out the lastest main (1e165b9c5f91deed4646c5cfc9e4f563b949c8ca) and build it successfully after commenting out numba in setup.py file. But ran into errors during the pytest test/ step with python3.10.

______________ ERROR collecting test/test_verilogae/test_get_param_list.py _______________
ImportError while importing test module '/home/dilawar/Work/GITHUB/FOSS/DMT-core/test/test_verilogae/test_get_param_list.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/test_verilogae/test_get_param_list.py:5: in <module>
    from DMT.core import MCard
DMT/core/__init__.py:47: in <module>
    from pint import UnitRegistry
/home/dilawar/.local/lib/python3.10/site-packages/Pint-0.19.1-py3.10.egg/pint/__init__.py:30: in <module>
    ???
/home/dilawar/.local/lib/python3.10/site-packages/Pint-0.19.1-py3.10.egg/pint/registry.py:69: in <module>
    ???
/home/dilawar/.local/lib/python3.10/site-packages/Pint-0.19.1-py3.10.egg/pint/parser.py:21: in <module>
    ???
E   ModuleNotFoundError: No module named 'pint._vendor'
================================ short test summary info =======

Looks like an issue with the pint library and is specific to Python3.10. Because I could not reproduce it with python3.8 on a Debian box, I did not open any issue on the dmt-core repo. I did not try python3.9.

I opened an issue on the pint repo hgrecco/pint#1511.

I am running on ubuntu 18:

(DMT_3.10) mario@mario-MS-7885:~/Documents/work/dmt/dmt$ python
Python 3.10.4 (main, Mar 24 2022, 16:14:25) [GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pint
>>> 
(DMT_3.10) mario@mario-MS-7885:~/Documents/work/dmt/dmt$ pip show pint
Name: Pint
Version: 0.19.1
Summary: Physical quantities module
Home-page: https://github.com/hgrecco/pint
Author: Hernan E. Grecco
Author-email: hernan.grecco@gmail.com
License: BSD
Location: /home/mario/venv/DMT_3.10/lib/python3.10/site-packages
Requires: 
Required-by: DMT-core

It seems to be a very specific issue. I hope they can solve it inside of Pint

lucydot commented 2 years ago

Thanks for the excellent work you are doing on the reviews @dilawar @phoebe-p.

RE: the related publications section of the paper @miesli: I agree it looks a little awkward with the list of brackets. I suggest a slight re-wording: The project has been used in the following publications --> The project has been used in the following contexts (or you may think of a nicer word to put instead of contexts) and then the bullet pointed list but with description first, followed by reference.

Please also re-format DMT has been mentioned by Muller et al (2021) so that the reference includes the author. I'm not quite clear what is meant be mentioned - would introduced, discussed or cited be a more accurate way of putting it?

miesli commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

miesli commented 2 years ago

I reformulated the "related publications" and completely agree, this looks way less awkward. Thank you for the tips.

dilawar commented 2 years ago

@miesli I am in the last phase of the review. The codebase quality and the documentation are excellent and the package is very capable. I'll post the summary of the review later.

I feel the following point, though not important for this review, will be useful to consider in the future.

dilawar commented 2 years ago

@miesli

From the paper:

DMT implements a bullet-proof grammar for naming electrical quantities for solving this problem. During data import all data columns are translated to this grammar.

Can you point me to the file/function where this grammar is implemented?

phoebe-p commented 2 years ago

@miesli Agreed that the new list format for the paper looks better - one note, when you are citing multiple publications at once such as line 101-102, you should group the references: [@citation1; @citation2, @citation3]

miesli commented 2 years ago

Sorry, for the long response time, I took the weekend to set up Ubuntu 22.04, as I started to run in more and more strange dependency errors in my old Ubuntu 18 install. This is not because of DMT, but more because I tried many things on my pc (like building llvm11 myself, which I can not recommend doing Oo).

So back to buisness:

@phoebe-p Thanks for the tip regarding the proper grouping of references. I changed the paper accordingly.

@dilawar

phoebe-p commented 2 years ago

Hi @miesli, I'm nearly done with the review, and I agree with @dilawar that the package is good and the issues I describe below are not blockers to acceptance, but something to keep in mind for the usability of the package by others.

Somewhat in line with what @dilawar has been discussing, there are so many dependencies and libraries which have to be installed etc. especially to get the other dependencies (ngspice, Xyce, Hdev, plotting implementations etc.) to work, some of which are not documented. I know from experience that this is a pain and even just sticking with Ubuntu (I am using version 20.04), seemingly every single time something will be slightly different, and it will be almost impossible to write fool-proof installation instructions... However, all this will be very confusing even for someone who is quite good at Python but doesn't have experience with building things from source.

Currently, I have (attempted to) install all the dependencies apart from Xyce, but am not able to easily run any of the three examples in the documentation - running_a_simulation.py requires Xyce, so since I didn't attempt to install it, let's leave that aside for now. basic_data_handling.py fails due to ongoing issues I am having with PyQtGraph:

qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.

I attempted to resolve this, I am sure it's solvable but I couldn't figure it out quickly. If I change the plotting to use matplotlib instead, I STILL get an error to do with using LaTeX in the labels (this one is puzzling, because I have had this error before but managed to resolve it and I frequently use LaTeX strings in my own figures so I have no idea what is causing the error), the gist of which is:

RuntimeError: latex was not able to process the following string:
b'lp'

Finally, because of a similar issue as described here, the read_in_measurement_data.py example initially failed because I was trying to run it from the 'examples' folder but it only works if being run from the dmt-core folder, since it looks for the 'test' folder. This should be very easy to fix.

You are very explicit in the documentation that people should expect to invest time into understanding how DMT works before it becomes useful to them, that it requires dependencies to be installed etc. (and many of these issues are outside of your control anyway because they have to do with other packages) but I think there are still things which can be improved to make it slightly more user-friendly (for example, maybe allowing a plotting option which does not require LaTeX at all).

miesli commented 2 years ago

Hi @phoebe-p, thank you for the remarks and yes I can only agree. This is one reason why we want to make this toolkit open source now (even if it is not really mature). We need as much testing on different machines by users as possible. The issue here is, that you have to be interested in programming, the used OS and the wanted interface at the same time. DMT only starts to pay off after a serious invest of time. But in our field this is always the case. Any program/tool you are using needs a lot of work to get used to it.

The advantage of DMT is, that you access the wanted tool using python and are able to run any calculations etc. in a general way there. This makes transitions between the different tools afterwards much less painful. Also, it allows transferring scripts for colleagues or even publications, this is something very difficult in our field currently. So we hope that the initial time invest pays off in the long run.

I will try to mention this issue somehow in the introduction.

Regarding your concrete issues:

Thank you both for working through the installation issues.

dilawar commented 2 years ago

I recommend that DMT-core is accepted for publication. The codebase is of good quality and easily extendible making it suitable for building on top of it. For a few checkpoints such as State of the field and Statement of need, I am mostly relying on @phoebe-p expertise since I don't have much experience in this field currently (I did my master's in VLSI Tech in 2010).

Currently, ideal DMT-core users seem to be Python programmers with domain expertise. Given the nature of tooling in this domain (I have a limited experience), the dependency list, and the experiences shared by my co-reviewer @phoebe-p, it feels like that currently the users of DMT-core also need to know a fair bit about installing and compiling dependencies on their platform.

Minor suggestions

Wishlist

lucydot commented 2 years ago

Hi @dilawar thank you for completing your review so quickly and thoroughly. Can I ask if you were able to run through all of the three examples in the DMT documentation (and so also install of the required dependencies on your system)?

dilawar commented 2 years ago

Leaving a note here: https://github.com/snmishra/xyce-docker/blob/master/Dockerfile shows how to install Xyce on Ubuntu-20.04. Its pretty painful!

miesli commented 2 years ago

Thank you, @dilawar, for the positive review, although the installation was not easy.

The suggestions you added are again very helpful, and we may use some of them in one or the other way.

Supporting the different platforms is not easy with such a high level project, which depends on so many other programs. Hopefully, there will be people interested enough in the project to install it with us and help us to improve the situation.

The issue again is, that the other software packages have options, and we currently need a specific setting to get DMT up and running properly. Here is the installation bash script I used recently to configure and install Xyce on my newly installed Ubuntu 22: install_xyce_dynamic.txt The issue is, that the needed version of Xyce 7.3 is not the one from the GitHub repo, but from the release homepage. Sadly, there only the current version (7.4) is available, and I can not find the proper version anywhere publically accessible. This is on our agenda and with enough demand we may even talk to Sandia directly, but for now and keeping in mind, that we may be able to push a big update for the version 7.5 to them, it seemed not to be worth the trouble keeping in mind, that ngspice is much easier to use.

I already implemented your suggestion for the standardized configuration file location. The new paths are:

This solves an Issue @phoebe-p had at the same time. There is no need to rename the file anymore if the default config is copied.

dilawar commented 2 years ago

Hi @dilawar thank you for completing your review so quickly and thoroughly. Can I ask if you were able to run through all of the three examples in the DMT documentation (and so also install of the required dependencies on your system)?

Hi @lucydot. Not all of them. 2 of them passed but 2 didn't due to dependencies misbehaving. I have unchecked an item from the list that deals with examples. I am going to open a couple of issues on the repo regarding this and wait till those are resolved.

dilawar commented 2 years ago

Hi @miesli . Thanks for the script. My old laptop wets its pant while building Xyce. I managed to borrow a powerful machine from my neighbour and manage to build Xyce:7.4.0. I created a docker image (repo) for reuse. I see that you are using version 7.3.0.

On top of this image, I created another image to build and test DMT-core. The code is here https://gitlab.com/dilawar/dmt-core/-/tree/joss (on branch joss). I've added two extra files: Dockerfile and a Makefile.

Executing command make examples will download the docker image, install DMT-core along with required dependencies and run the example. Here is the sample run on my machine (this is based on ArchLinux. Sorry about not using Debian/Ubuntu for this. Its just easier to develop on Arch with bleeding edge!)

Successfully built 51d233cde953
Successfully tagged dilawars/joss-dmt:latest
docker run -it dilawars/joss-dmt python ./doc/source/examples/running_a_simulation.py
-----------------------------------------------------------------------
DMT Copyright (C) 2022 SemiMod
This program comes with ABSOLUTELY NO WARRANTY.
DMT_core is free software, and you are welcome to redistribute it.
-----------------------------------------------------------------------
Finished building hicuml2v2p4p0_xyce.va in 0.56s
Compiling Xyce plugins : |                                                  | 0% completed

 Prepared a simulation in the folder:
/root/.DMT/simulation_results/xyce_069e16046adde515be29e09b4014f72b/gummel_f248342e035707d62888242f14d9caac
Preparing Simulations : |██████████████████████████████████████████████████| 100% completed

DMT is now simulating!                                                                                               finished: 1 of 1:[####################]                                                                              -------------------------------                                                                                      | sim_n | pid        | dt     |                                                                                      -------------------------------                                                                                      |   x   |     x      |  0.0   |                                                                                      |   x   |     x      |  0.0   |                                                                                      |   x   |     x      |  0.0   |                                                                                      |   x   |     x      |  0.0   |                                                                                      -------------------------------                                                                                                                                                                                                           Reading in the results:
Read: dut: xyce_069e16046adde515be29e09b4014f72b, sweep: gummel_f248342e035707d62888242f14d9caac
Simulation of DuT xyce_069e16046adde515be29e09b4014f72b with sweep gummel_f248342e035707d62888242f14d9caac failed.
Simulation folder: /root/.DMT/simulation_results/xyce_069e16046adde515be29e09b4014f72b/gummel_f248342e035707d62888242f14d9caac
The simulation command is
Xyce -plugin /root/.DMT/simulation_results/xyce_plugins/3f7640fd04d68cb6862f7ee9782b3821.so xyce_circuit.cirNetlist error: Failed to load plugin
 /root/.DMT/simulation_results/xyce_plugins/3f7640fd04d68cb6862f7ee9782b3821.so:
 cannot open shared object file: No such file or directory
Simulation aborted due to error.  There are 0 MSG_FATAL errors and 1 MSG_ERROR
 errors
*** Xyce Abort ***
Simulation aborted due to error.  There are 0 MSG_FATAL errors and 1 MSG_ERROR
 errors

*** Xyce Abort ***

Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/DMT_core-1.5.0rc5-py3.10.egg/DMT/core/dut_view.py", line 643, in get_data
    return self._data[key]
KeyError: 'T300.00K/gummel_f248342e035707d62888242f14d9caac/iv'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/dmt-core/./doc/source/examples/running_a_simulation.py", line 122, in <module>
    data = dut.get_data(sweep=sweep)
  File "/usr/lib/python3.10/site-packages/DMT_core-1.5.0rc5-py3.10.egg/DMT/core/dut_view.py", line 646, in get_data
    return self._data[self.join_key("", key)]
KeyError: '/T300.00K/gummel_f248342e035707d62888242f14d9caac/iv'
make: *** [Makefile:11: examples] Error 1
[admin@ip-172-26-7-193 DMT-core (joss)]$

Xyce and buildxyceplugin are called but the shared object file had issue.

I've also upload this image to docker hub in case you want to reproduce my setup. The uploaded image checked out commit 4600e0597c8057734c0351bf538d5941a01b050c from https://gitlab.com/dilawar/dmt-core/-/tree/joss

docker run -it dilawars/joss-dmt:latest bash
[root@0f316a97c866 dmt-core]# pwd
/dmt-core
[root@0f316a97c866 dmt-core]# ls
build      codemeta.json  DMT                doc         LICENSE  Makefile        README.md  test
CHANGELOG  dist           DMT_core.egg-info  Dockerfile  logs     pyproject.toml  setup.py
[root@0f316a97c866 dmt-core]#

For other examples that creates GUI, you may have to launch docker with X11 support. I found very helpful note here https://wiki.ros.org/docker/Tutorials/GUI.

@phoebe-p I also encountered this issue qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.. In my case, X11 forwarding was not working properly (over ssh).

phoebe-p commented 2 years ago

@dilawar hmm, I was not using any kind of remote connection so I guess the source of the QT error is something else for me.

@lucydot as I said above I was also not able to run all the examples for issues that are not really due to DMT-core. It is not feasible for my install Xyce at the moment (the main laptop I am using at the moment is a Macbook, which was causing too many problems, and I have access to Linux (Ubuntu) only on my old laptop where it is installed on a partition of the hard drive which is almost full so installing big packages isn't possible. Obviously it is not ideal, but I don't really have the bandwidth to get a better Linux setup going and install Xyce/figure out my issues with LaTeX just for this review. As I said above it would be good to have some plotting capabilities which do not rely on LaTeX for this reason. I don't know if both reviewers being able to run the examples is required for acceptance. In this case because the issues are not really with DMT-core, and it is by design a high-level software with many dependencies, I don't think it should be a blocker to acceptance.

@miesli on the "Community guidelines" review point, hopefully this would be obvious to people who are attempting to use DMT-core but in my experience managing a package, many people do not know about/are resistant to using GitHub issues... So in line with this point, adding some explicit instructions on what to do if you face an issue (open an issue on GitLab, or maybe email the developers if you prefer that) would be useful. I don't know about GitLab, but on GitHub you can make different 'issue templates', for example in one of my packages I have 'Bug report', 'Feature request' and 'Question', and it will give some prompts for people to describe their issue with useful information.'

Apart from this minor point, I have completed the items on the checklist and am happy for this submission to be accepted.

miesli commented 2 years ago

@phoebe-p the template for issues and merge-requests are a good idea. I dug into the stuff of gitlab regarding this and added some. I updated the repo readme to directly link to the corresponding templates. This hopefully lowers the bar for that. I also added a similar section to the welcome page in the documentation.

@dilawar yes, the buildxycepluginis the same Issue as I have when I tried to switch to the githhub release of Xyce. I was not able to resolve this issue, but I also did not spend too much time on that. I added you to our "master" repository, where we have uploaded the needed xyce source code. This together with the Dockerfile there will enable you to run the examples like they are run in the CI environment. This is of course not a solution for everyone. Currently, we would recommend everyone to use ngspice as long as no Verilog-A support is needed. If the demand for Verilog-A support is large, we will have to talk to Sandia if we can "publish" the version 7.3 given in the repo. We can do that as we are in contact with them anyway, but we hoped we can directly link to the released DMT JOSS paper for that to show the demand, but maybe the paper under review is already enough...

The alternative idea is to wait until our new Verilog-A compiler is implemented there, this should be better than the current solution using ADMS...

I am looking forward to the issues in the repo to hopefully solve the issues, so you can run the examples successfully.

dilawar commented 2 years ago

Thanks for adding me to repo. I'll use it as the last resort. I made some progress and at this point, I don't want this effort to go to waste. I'll try a few more days to get Xyce:7.4, please bear with me. Also, I'd be nice to have a publicly available image.

I am reporting my progress here. I think I am pretty close to getting DMT-core working with Xyce:7.4 🤞🏼

I looked into logs. The errors reported previously were due to missing dependencies bc and adms. After adding them to the image, I was able to run the Xyce:7.4 successfully, but not the example. 

[admin@ip-172-26-7-193 DMT-core (joss)]$ make examples
docker run -it dilawars/joss-dmt:latest python ./doc/source/examples/running_a_simulation.py
-----------------------------------------------------------------------
DMT Copyright (C) 2022 SemiMod
This program comes with ABSOLUTELY NO WARRANTY.
DMT_core is free software, and you are welcome to redistribute it.
-----------------------------------------------------------------------
Finished building hicuml2v2p4p0_xyce.va in 0.56s
Compiling Xyce plugins : |                                                  | 0% completed

 Prepared a simulation in the folder: 
/root/.DMT/simulation_results/xyce_069e16046adde515be29e09b4014f72b/gummel_f248342e035707d62888242f14d9caac
Preparing Simulations : |██████████████████████████████████████████████████| 100% completed

DMT is now simulating!                                                                     finished: 1 of 1:[####################]                                                    -------------------------------                                                            | sim_n | pid        | dt     |                                                            -------------------------------                                                            |   x   |     x      |  0.0   |                                                            |   x   |     x      |  0.0   |                                                            |   x   |     x      |  0.0   |                                                            |   x   |     x      |  0.0   |                                                            -------------------------------                                                                                                                                                       Reading in the results:
Read: dut: xyce_069e16046adde515be29e09b4014f72b, sweep: gummel_f248342e035707d62888242f14d9caac
Simulation of DuT xyce_069e16046adde515be29e09b4014f72b with sweep gummel_f248342e035707d62888242f14d9caac failed.
Simulation folder: /root/.DMT/simulation_results/xyce_069e16046adde515be29e09b4014f72b/gummel_f248342e035707d62888242f14d9caac 
The simulation command is
Xyce -plugin /root/.DMT/simulation_results/xyce_plugins/3f7640fd04d68cb6862f7ee9782b3821.so xyce_circuit.cir
*****
***** Welcome to the Xyce(TM) Parallel Electronic Simulator
*****
***** This is version Xyce Release 7.4-opensource
***** Date: Sat Apr 30 02:46:30 UTC 2022

***** Executing netlist xyce_circuit.cir

***** Reading and parsing netlist...
***** Setting up topology...

Netlist warning: Voltage Node (N_T) connected to only 1 device Terminal
Netlist warning: Voltage Node (N_TNODE) connected to only 1 device Terminal
Netlist warning: Voltage Node (N_TNODE) does not have a DC path to ground
***** Device Count Summary ...
       Q level 234 (HICUM v2.4.0)             1
       R level 1 (Resistor)                   2
       R level 3 (Resistor)                   3
       V level 1 (Independent Voltage Source) 2
       ----------------------------------------
       Total Devices                          8
***** Setting up matrix structure...
***** Number of Unknowns = 22
***** Initializing...

Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/DMT_core-1.5.0rc5-py3.10.egg/DMT/core/dut_view.py", line 643, in get_data
    return self._data[key]
KeyError: 'T300.00K/gummel_f248342e035707d62888242f14d9caac/iv'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/dmt-core/./doc/source/examples/running_a_simulation.py", line 122, in <module>
    data = dut.get_data(sweep=sweep)
  File "/usr/lib/python3.10/site-packages/DMT_core-1.5.0rc5-py3.10.egg/DMT/core/dut_view.py", line 646, in get_data
    return self._data[self.join_key("", key)]
KeyError: '/T300.00K/gummel_f248342e035707d62888242f14d9caac/iv'
make: *** [Makefile:13: examples] Error 1
[admin@ip-172-26-7-193 DMT-core (joss)]$

I can confirm like that dut._data is {} when the exception is thrown. Perhaps the simulation did not succeed and/or it didn't generate data?

I also looked into /root/.DMT/simulation_results/xyce_069e16046adde515be29e09b4014f72b/gummel_f248342e035707d62888242f14d9caac folder. File DC.csv has only one line.

[root@58db8d62a3a0 gummel_f248342e035707d62888242f14d9caac]# cat DC.csv 
TIME,TEMP,V(N_E),V(N_B_FORCED),V(N_S),V(N_C),V(N_T),V(N_C_FORCED),V(N_TNODE),V(N_B),I(RI_E),I(VV_C),I(RI_B),I(RI_C),I(VV_B),AC_SWITCH
0.00000000e+00,2.68500000e+01,0.00000000e+00,5.00000000e-01,2.09192882e-56,0.00000000e+00,0.00000000e+00,0.00000000e+00,0.00000000e+00,5.00000000e-01,1.16013674e-09,9.27457890e-10,-2.08759463e-09,9.27457890e-10,-2.08759463e-09,0.00000000e+00

Netlist generated looks Ok https://pastebin.com/raw/u909Hynt . Do you think it's a minor issue or is caused by some regression in Xyce:7.4.0?

miesli commented 2 years ago

Hi @dilawar,

I'm quite impressed by your work. It seems to me, that you have come quite far.

There is one issue with the netlist, but this should not abort the simulation: 

Netlist warning: Voltage Node (N_T) connected to only 1 device Terminal
Netlist warning: Voltage Node (N_TNODE) connected to only 1 device Terminal
Netlist warning: Voltage Node (N_TNODE) does not have a DC path to ground

N_T and N_TNODE are the same, but named wrongly. I will correct this (also, there are other netlists with the same issue). But the thermal node is not relevant in this (or most) simulation setups. The simulation result is still correct.

The real issue does not show in the DMT call, only in the direct simulation call:

[root@3bca57b39169 gummel_f248342e035707d62888242f14d9caac]# Xyce -plugin /root/.DMT/simulation_results/xyce_plugins/3f7640fd04d68cb6862f7ee9782b3821.so xyce_circuit.cir

*****
***** Welcome to the Xyce(TM) Parallel Electronic Simulator
*****
***** This is version Xyce Release 7.4-opensource
***** Date: Mon May 02 08:56:21 UTC 2022

***** Executing netlist xyce_circuit.cir

***** Reading and parsing netlist...
***** Setting up topology...

Netlist warning: Voltage Node (N_T) connected to only 1 device Terminal
Netlist warning: Voltage Node (N_TNODE) connected to only 1 device Terminal
Netlist warning: Voltage Node (N_TNODE) does not have a DC path to ground
***** Device Count Summary ...
       Q level 234 (HICUM v2.4.0)             1
       R level 1 (Resistor)                   2
       R level 3 (Resistor)                   3
       V level 1 (Independent Voltage Source) 2
       ----------------------------------------
       Total Devices                          8
***** Setting up matrix structure...
***** Number of Unknowns = 22
***** Initializing...

/usr/include/c++/11.2.0/bits/stl_vector.h:1045: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](std::vector<_Tp, _Alloc>::size_type) [with _Tp = int; _Alloc = std::allocator<int>; std::vector<_Tp, _Alloc>::reference = int&; std::vector<_Tp, _Alloc>::size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Aborted (core dumped)

The c++ lib issue is not outputted into the stdout (it seems). Googling that issue, leads to a GitHub issue: https://github.com/otfried/ipe-issues/issues/396

There seems to be an issue with -D_GLIBCXX_ASSERTIONS. I checked the sources, I know of, in which this should be unset, but I did not find a single mention of it in the trilinos and xyce building chain (maybe I missed one). Do you know more about this issue?

dilawar commented 2 years ago

Thank you @miesli for digging a bit deeper.

@lucydot I understand that both reviewers can't run all the examples at this moment. I think I am almost there but due to a low-level issue with dependencies, I also couldn't run the examples on my Linux box. I'll try again with inputs provided by @miesli but I am also a bit low on time. I agree with @phoebe-p that "the issues are not really with DMT-core, and it is by design a high-level software with many dependencies". What do you recommend at this moment?

One suggestion is that authors can modify examples with ngspice as the default simulator. ngspice is easily available on various platforms.

lucydot commented 2 years ago

Thanks @phoebe-p and @dilawar for your time and expertise on this review. I can see you have both already invested a lot of time into installation alone.

@miesli what would be lost from modifying the examples so that they work with ngspice as the default simulator? I can imagine that if @phoebe-p and @dilawar are still facing issues after extended attempts, it will be the same story for many of your potential users - would you consider adjusting the examples so that they can be more easily followed?

miesli commented 2 years ago

I have to fully agree. Thank you @phoebe-p and @dilawar for your time and patience with our project!

@lucydot In fact, from the 3 examples in the documentation:

https://dmt-development.gitlab.io/dmt-core/examples/basic_data_handling.html https://dmt-development.gitlab.io/dmt-core/examples/running_a_simulation.html https://dmt-development.gitlab.io/dmt-core/examples/read_in_measurement_data.html

Only "running_a_simulation" uses a circuit simulator. There we show the capabilities to switch between simulators using only one line of code:

# init an Xyce Dut
dut = DutXyce(None, DutType.npn, modelcard, nodes="C,B,E,S,T", reference_node="E")
# DMT uses the exact same interface for all circuit simulators, e.g. the call for a ngspice simulation would be:
# dut = DutNgspice(None, DutType.npn, modelcard, nodes="C,B,E,S,T", reference_node="E")
# isn't it great?!?

So it is a matter of switching the lines. This is in fact one exact use case for DMT and its different back-ends.

Best regards

dilawar commented 2 years ago

I managed to run examples by changing the default simulator to ngspice as suggested in https://github.com/openjournals/joss-reviews/issues/4298#issuecomment-1116127721. 🥳

I again run into qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found. problem as reported by @phoebe-p on a different machine. This time the solution was executing xhost + on the host machine. It disabled some security checks and allowed pyqtgraph to access X windows system. 

$ xhost +
access control disabled, clients can connect from any host
$ /usr/bin/docker run -it -e DISPLAY=$DISPLAY -v /tmp/.X11-unix:/tmp/.X11-unix dilawars/joss-dmt:latest python ./doc/source/examples/basic_data_handling.py
lucydot commented 2 years ago

Thanks for the update @dilawar.

Software examples/tutorials are often useful for assessing Have the functional claims of the software been confirmed? in the review checklist - so it's great news that you have been able to execute them all :)

@miesli it seems to me that some of the installation/dependancies lessons-learnt from @dilawar and @phoebe-p might be useful to your user community. I can see you have a page in your documentation "More Installation Issues' - perhaps some of the tips from this review could be added to this section (or another) of your documentation. For example, the problem above relating to the qt.qpa.plugin.

miesli commented 2 years ago

@lucydot yes, this is a good point. I will collect all the stuff @phoebe-p and @dilawar found in the documentation. But this will not happen this week, as my calendar is already a little dense. Hopefully I can stuff it in at the start of next week. Is this ok for you? I will have to sort the different issues to regarding the OS and if it is run inside a docker container. I will also link to the great container @dilawar has put together.

What other steps for the publication are remaining?

Furthermore, I heard that Sandia is currently preparing their next release (7.5) and it will be finished soon. As soon as it is out, I will check if I can build their code for dynamic loading using ADMS in a container. Compatibility to our new compiler may be part of the release next year, in the best case, so I think it will be worthwhile to update the build-chain.

lucydot commented 2 years ago

Hi @miesli please keep us updated here with your progress with updating the documentation.

Once that it is done I will do final check through of the paper (spelling, grammar, citations etc). There will also be some small admin steps (including creating release on Zenodo or similar) before passing on to the editors-in-chief for publication.

miesli commented 2 years ago

Ah, I just pushed the update of our installation documentation. There I added the "xcb" error and also an explanation and link to @dilawar 's Dockerfile. As soon as the next Xyce release is out, I will try to implement it and also add it to the Docker. In the best case, we can run our CI using the new container then.

I also resolved the matplotlib latex issue . It is now possible to plot using Plot.plot_py(use_tex=False). This should be self-explanatory.

lucydot commented 2 years ago

@editorialbot generate pdf

lucydot commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1109/MS.2010.152 is OK
- 10.2172/1826862 is OK
- 10.25080/majora-92bf1922-00a is OK
- 10.1109/MMM.2021.3117139 is OK
- 10.1109/JEDS.2020.3023165 is OK
- 10.1109/TED.2021.3051552 is OK
- 10.1109/bcicts50416.2021.9682487 is OK
- 10.1109/bcicts50416.2021.9682485 is OK
- 10.1109/bcicts50416.2021.9682473 is OK
- 10.1109/JEDS.2020.3023165 is OK

MISSING DOIs

- None

INVALID DOIs

- None