[REVIEW]: NLMech: Implementation of finite difference/meshfreediscretization of nonlocal fracture models

whedon commented 3 years ago

Submitting author: @diehlpk (Patrick Diehl) Repository: https://github.com/nonlocalmodels/NLMech Version: v0.1.0 Editor: @Kevin-Mattheus-Moerman Reviewer: @vijaysm, @chennachaos Archive: 10.6084/m9.figshare.16688695.v1

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

Status badge code:

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

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

@vijaysm & @chennachaos, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @Kevin-Mattheus-Moerman 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 ✨

Review checklist for @vijaysm

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@diehlpk) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @chennachaos

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@diehlpk) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

diehlpk commented 3 years ago

@vijaysm I added some example to the README.md here

https://github.com/nonlocalmodels/NLMech/pull/140

how to

pull the docker image
run the docker image using the bash
and run one of the test case in 1D to take not too long to finish in the Docker image.

diehlpk commented 3 years ago

3.) The installation instructions are quite clear. However, the functionality documentation is severely lacking. You need to add a page or two describing the fundamental equations that the library solves, and a brief overview of the method(s) used.

@chennachaos We have all this in the methods paper

Diehl, P., Jha, P. K., Kaiser, H., Lipton, R., & Lévesque, M. (2020). An asynchronous and task-based implementation of peridynamics utilizing HPX—the C++ standard library for parallelism and concurrency. SN Applied Sciences, 2(12), 1-21.

Jha, PK, Lipton R. “Numerical convergence of finite difference approximations for state based peridynamic fracture models.” Computer Methods in Applied Mechanics and Engineering, 1 July 2019, 351(1), 184 - 225. Link

I am happy to mention this in the documentation, however, I do not think that it is necessary to add all these equations to the documentation.

diehlpk commented 3 years ago

1.) Although the important details seem to be there, the documentation is spanned over two different websites. Installation instructions are in the new documentation and the examples and other details are in the old site. Some details are on both sites. I recommend combining these two.

@chennachaos This is on purpose to only have the light-weight documentation, e.g. installation, running the code, and details about the configuration files, in the code repo (https://github.com/nonlocalmodels/NLMech). The heavy documentation, e.g. images for the examples, are outsourced to the webpage's (https://nonlocalmodels.github.io/) repo (https://github.com/nonlocalmodels/nonlocalmodels.github.io). The doxygen documentation is published there as well (https://nonlocalmodels.github.io/documentation/).

We do not think that images should be inside the code repo, especially if you will submit the code as a Fedora or Ubuntu package to their package manager it is not nice if you have images shipped with the source code.

diehlpk commented 3 years ago

7.) Add documentation on preparing the input files in the .yml format, together with the explanations of different blocks and the parameters used in each block in the input file.

@chennachaos We started to add the documentation for the configuration files. I added the mesh generation files here

https://github.com/nonlocalmodels/NLMech/pull/141

We will add more for the simulation, however, that will take some time.

Kevin-Mattheus-Moerman commented 3 years ago

@vijaysm, @chennachaos it looks like the author replied to some of the issues/questions you raised :point_up:. Would you be able to pick up the review and respond with the key issues you feel still need work? Thanks again for your help!!!!!!

vijaysm commented 3 years ago

@diehlpk Some review comments on the paper.

Line 19: Replace "I.e." with "i.e.,"
Line 21: Missing commas: "radius $\delta$, centered at x, where"
Line 44: Correct this: "Figure Figure 2"
Figure 2: Caption. Fix "Adapted"
It may help to talk about the theory and the software implementation of it separately, rather than mixing them under the same section. Specifically, look at lines 53-56. My suggestion here is to combine this paragraph with the "Applications" section and make it "Software Implementation and Applications" for better clarity.
Line 74: Missing comma: "from an open source perspective, only two"
Line 79: No need for a bracket here: "we refer to Diehl et al., 2020."

diehlpk commented 3 years ago

@whedon generate pdf from branch joss

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss. Reticulating splines etc...

whedon commented 3 years ago

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

diehlpk commented 3 years ago

@whedon generate pdf from branch joss

whedon commented 3 years ago

Attempting PDF compilation from custom branch joss. Reticulating splines etc...

whedon commented 3 years ago

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

diehlpk commented 3 years ago

@diehlpk Some review comments on the paper.

* Line 19: Replace "I.e." with "i.e.,"

* Line 21: Missing commas: "radius $\delta$, centered at x, where"

* Line 44: Correct this: "Figure Figure 2"

* Figure 2: Caption. Fix "Adapted"

* It may help to talk about the theory and the software implementation of it separately, rather than mixing them under the same section. Specifically, look at lines 53-56. My suggestion here is to combine this paragraph with the "Applications" section and make it "Software Implementation and Applications" for better clarity.

* Line 74: Missing comma: "from an open source perspective, only two"

* Line 79: No need for a bracket here: "we refer to Diehl et al., 2020."

@vijaysm Thanks for the comments. The above build of the paper addresses all of your comments.

vijaysm commented 3 years ago

Excellent. Thanks for quickly addressing the comments. @diehlpk I'll be able to test installation, examples etc in the next two days and can give you my final set of comments. Sorry about the delay.

chennachaos commented 3 years ago

Thank you @diehlpk for your responses!

2) The link to "10.1007/s42452-020-03784-x" in the paper "Please cite following publications, if you use the code" is broken.

chennachaos commented 3 years ago

1.) This is fine in that case.

2.) The link to "10.1007/s42452-020-03784-x" in the paper "Please cite following publications, if you use the code" is broken.

3.) I understand that the equations are provided in the paper but the online documentation should contain some basic equations, if not all, so that the reader can easily relate to what is being solved without needing to refer to the papers all the time.

4.) Not addressed.

5.) I don't think it is sufficient to refer to the papers for the accuracy studies. The documentation must describe a few examples; this should include some comparisons with literature (experiment/numerical/analytical). You may use the same graphs or other comparisons from your papers.

6.) I think these 2D geometries are complex enough. It is understandable that 3D problems are computationally more demanding and that it will require considerable effort to parallelism the code for distributed computing. However, I believe that the code should be able to solve some simple 3D problems, even without parallelisation.

7.) This (documentation on preparing input files) needs to be completed before the submission can be recommended for publication. Otherwise, the users will struggle to use the library.

8.) This is good.

9.) Thanks for the clarification! It is understandable that parallelization for distributed memory architectures takes time. Until then, please mention in the documentation that the current capability is limited to thread-based parallelism.

diehlpk commented 3 years ago

2.) The link to "10.1007/s42452-020-03784-x" in the paper "Please cite following publications, if you use the code" is broken.

@chennachaos That is fixed now.

diehlpk commented 3 years ago

9.) Thanks for the clarification! It is understandable that parallelization for distributed memory architectures takes time. Until then, please mention in the documentation that the current capability is limited to thread-based parallelism.

@chennachaos That was added to the Readme.md and the main page of the documentation.

diehlpk commented 3 years ago

3.) I understand that the equations are provided in the paper but the online documentation should contain some basic equations, if not all, so that the reader can easily relate to what is being solved without needing to refer to the papers all the time.

@chennachaos I added the basic equations showed in the JOSS paper to the documentation and referenced them in the Readme.md and the index.html of the documentation.

See https://github.com/nonlocalmodels/NLMech/pull/144

Meanwhile, the webpage is deployed:

https://nonlocalmodels.github.io/documentation/md_content_equations.html

diehlpk commented 3 years ago

5.) I don't think it is sufficient to refer to the papers for the accuracy studies. The documentation must describe a few examples; this should include some comparisons with literature (experiment/numerical/analytical). You may use the same graphs or other comparisons from your papers.

We can not easily reuse the images from the papers, since the copyright was transferred to the publisher. So we would need to be careful here. In addition, the documentation is generated using doxygen and the support of equations is limited there.

We have two examples where we showed that we recover properties from linear elasticity:

All other benchmarks are very technical, and I do not think there is any benefit to copy the results from the paper to the documentation, since all papers are available as preprints and the reader can read there the exact same information.

diehlpk commented 3 years ago

7.) This (documentation on preparing input files) needs to be completed before the submission can be recommended for publication. Otherwise, the users will struggle to use the library.

@chennachaos All the common attributes which are essential to get started with the code are added. Meaning, all tested and user-friendly attributes are documented. However, experimental and under current development are not yet there, but will later be added once the features are considered as stable.

https://github.com/nonlocalmodels/NLMech/pull/145

diehlpk commented 3 years ago

6.) I think these 2D geometries are complex enough. It is understandable that 3D problems are computationally more demanding and that it will require considerable effort to parallelism the code for distributed computing. However, I believe that the code should be able to solve some simple 3D problems, even without parallelisation.

@chennachaos

Here, all data structures and algorithms are implemented to do 3D simulations. However, the geometrical functions, like applying boundary conditions not on a line or rectangle, are missing for 3D. We intend to add these features once we start the parallel implementation and add the load balancing as described in

https://arxiv.org/abs/2102.03819

From my experience, 3D simulations need fully distributed computations to run some complex geometry since the amount of nodes gets really large.

For example, in this paper [0] they got good agreements with the experimental data using > 4,173,281 discrete nodes in a small cubic. So I still do not think that this is feasible for a single node.

The specialty of the code is to have the softening models implemented where we have some numerical analysis which other PD models do not have. We're still exploring these in two dimensions but will go on to three dimensions, if we have a better understanding of crack and fracture in two dimensions.

[0] https://doi.org/10.1016/j.petrol.2016.05.032

diehlpk commented 3 years ago

4.) The documentation contains some good numerical examples. But the description of these numerical examples needs improvement. For each example, add the problem background, the objective, problem setup and the parameters to be studied etc. Also, describe different figures presented in the results section of examples provided.

@chennachaos

We made clear that the two examples with the code do not have any background. They were just there to make the code's logo.
Most of these examples were used in publications, and we just presented this examples here. We added the corresponding publications and linked the YAML files, so users can experience the full file and adapt it for their usage.

diehlpk commented 3 years ago

@chennachaos I addressed all your open remarks, please have a look.

Kevin-Mattheus-Moerman commented 3 years ago

@chennachaos looks like @diehlpk implemented more requested changes. Are you able to review these? Thanks.

chennachaos commented 3 years ago

Hi @diehlpk, thank you for the clarifications and updates to the documentation. I think the documentation is good now. I am going to check the installation and tests in the next couple of days and will provide you with my comments by the end of the week.

Kevin-Mattheus-Moerman commented 3 years ago

@vijaysm, @chennachaos can you give an update on review progress? Thanks again for your help!

chennachaos commented 3 years ago

Hi @Kevin-Mattheus-Moerman. Apologies for the delay. I have had difficulties in building the library. I will check again today and ask for help if the issues persist.

chennachaos commented 3 years ago

Hi @diehlpk, I am having difficulties installing the library. The problem is with the Blaze and Blaze_Iterative libraries. I was hoping that I could make use of the scripts provided but there are no files at the link Build Dependencies and Code on Ubuntu. I work with Ubuntu OS.

diehlpk commented 3 years ago

Hi @diehlpk, I am having difficulties installing the library. The problem is with the Blaze and Blaze_Iterative libraries. I was hoping that I could make use of the scripts provided but there are no files at the link Build Dependencies and Code on Ubuntu. I work with Ubuntu OS.

@chennachaos are you using the HPC Buildinfrastructure Scripts as shown in the documentation?

These scripts download and install all dependencies. Note that both libraries are headed only libraries.

Can you post any error message?

diehlpk commented 3 years ago

@chennachaos Have you seen this tutorial to compile the code

https://nonlocalmodels.github.io/documentation/md_content_install_instructions.html

vijaysm commented 3 years ago

@Kevin-Mattheus-Moerman I'm in a similar stage where my installation efforts for the TPL dependencies fail using the provided scripts. But I've been messaging @diehlpk who has provided a docker container with these pre-installed. I have tested the 1D and 2D examples using that container, and I am satisfied with the setup.

However, I'd like to follow the instructions to build NLMech again from scratch if possible. I have not yet tried building on a Fedora base system (I used Ubuntu as well) since that's what the scripts above use.

diehlpk commented 3 years ago

@vijaysm and @chennachaos could you please post some error messages and I might, can help you.

My institution's cluster runs on CentOS and my desktop on Fedora, so the scripts are not really tested. What Ubuntu version are you using? I can try to install the code using Docker.

diehlpk commented 3 years ago

@vijaysm and @chennachaos I pulled the Ubuntu latest Docker image from docker.io this morning.

And I had to install the following packages using apt-get

apt-get update
apt-get install build-essential
apt-get install git
apt-get install wget
apt-get install cmake
apt-get install libssl-dev
apt-get install libblas-dev liblapack-dev
apt-get install autoconf
apt-get install freeglut3-dev

Note that the scripts does not install any system packages for you. You have to make sure that you have some compiler, the build tools, and some most common system packages installed. I know that there is no package manager as we have in Python to detect missing dependencies.

After that I run

git clone https://github.com/nonlocalmodels/HPCBuildInfrastructure.git cd HPCBuildInfrastructure

and executed the following commands to build all dependencies:

./build-all.sh Release without-gcc cmake
./build-all.sh Release without-gcc hwloc
./build-all.sh Release without-gcc jemalloc
./build-all.sh Release without-gcc blaze
./build-all.sh Release without-gcc blazeIterative
./build-all.sh Release without-gcc boost
./build-all.sh Release without-gcc yamlcpp
./build-all.sh Release without-gcc hpx
./build-all.sh Release without-gcc vtk

All of them compiled for me out of the box, however, I fixed to small issue related to Docker in the build scripts.

I have to finish something else today, and will look into NLMech later this week.

It would really help, if you could post any error messages you have seen, since I could not see any error with the latest Ubunut verison.

chennachaos commented 3 years ago

Thank you @diehlpk! I will check again. I will start from scratch.

vijaysm commented 3 years ago

Thanks for the details @diehlpk and reverifying the instructions. I had my installation failures in some notes. I'll dig it up. I'm happy with the docker solution however, and if the script you had is more suitable for Fedora that should be fine as well. But I see you tested on Ubuntu now, and so I'll follow the new instructions.

One point that I explicitly remember was that I needed to install openssl and ssl-dev, which was not mentioned in the documentation. A dependency of HPX perhaps, but it's the curse of stacks.

Also, I was trying to use system installed cmake as I did not want to install it from source. This had some issues as well. Why do you need to build cmake from source? If you need the most up to date version, please specify that in documentation or README also. Otherwise, I'd recommend simplify instructions with just specifying - depends on CMake (>=3.1.0) for example.

I should have some more info for you later today, but it probably is outdated since the tests were done in May.

On Mon., Jul. 12, 2021, 16:25 Patrick Diehl, @.***> wrote:

@vijaysm https://github.com/vijaysm and @chennachaos https://github.com/chennachaos I pulled the Ubuntu latest Docker image from docker.io this morning.

And I had to install the following packages using apt-get

apt-get update

apt-get install build-essential

apt-get install git

apt-get install wget

apt-get install cmake

apt-get install openssl

apt-get install libssl-dev

apt-get install libblas-dev liblapack-dev

apt-get install autoconf

apt-get install freeglut3-dev

After that I run

git clone https://github.com/nonlocalmodels/HPCBuildInfrastructure.git cd HPCBuildInfrastructure

and executed the following commands to build all dependencies:

./build-all.sh Release without-gcc cmake

./build-all.sh Release without-gcc hwloc

./build-all.sh Release without-gcc jemalloc

./build-all.sh Release without-gcc blaze

./build-all.sh Release without-gcc blazeIterative

./build-all.sh Release without-gcc boost

./build-all.sh Release without-gcc yamlcpp

./build-all.sh Release without-gcc hpx

./build-all.sh Release without-gcc vtk

All of them compiled for me out of the box, however, I fixed to small issue related to Docker in the build scripts.

I have to finish something else today, and will look into NLMech later this week.

It would really help, if you could post any error messages you have seen, since I could not see any error with the latest Ubunut verison.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3020#issuecomment-878570883, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVEPJ3FIWLFIHZ5Y5C4RTTXNFSHANCNFSM4XIW5JHA .

diehlpk commented 3 years ago

Also, I was trying to use system installed cmake as I did not want to install it from source. This had some issues as well. Why do you need to build cmake from source? If you need the most up to date version, please specify that in documentation or README also. Otherwise, I'd recommend simplify instructions with just specifying - depends on CMake (>=3.1.0) for example.

HPX relies on a recent CMake version, and all codes on top of HPX need to use the same CMake version. I will update the minimal version on the README.md

diehlpk commented 3 years ago

One point that I explicitly remember was that I needed to install openssl and ssl-dev, which was not mentioned in the documentation. A dependency of HPX perhaps, but it's the curse of stacks.

Yes, that is a huge issue with things like openssl, autoconf, and so on. That is really operating system specific and there is no silver bullet to list all of them. For example apt-get install build-essential does not install autoconf, however, it seems to be installed on Fedora using dnf.

From my experience, these issues are for all C++ applications. I compiled HPX on many supercomputers, e.g. Summit, Piz Daint, and Cori. And we would not find some generic list of packages needed, since all clusters had a different software stack.

diehlpk commented 3 years ago

@chennachaos and @vijaysm

I added some new script to compile the code on Ubuntu

https://github.com/nonlocalmodels/buildscripts/blob/main/bash/buildAll_Ubuntu.sh

This might help.

chennachaos commented 3 years ago

Hi @diehlpk, the other dependencies you installed using apt-get install needs to be added to the list of dependencies on the installation page.

A major issue with the current support for building the library is that it seems to be built explicitly for HPC machines and, therefore, it is not compatible with the installation on personal computers. Use of module commands in the build scripts prevent installations on personal computers which usually do not have module management software installed on them.

Also, the idea of installing all the libraries separately is not good. This requires a significant amount of resources for compiling and building huge libraries like CMAKE and VTK.

There should be a provision to make use of libraries that are installed.

diehlpk commented 3 years ago

Use of module commands in the build scripts prevent installations on personal computers which usually do not have module management software installed on them.

@chennachaos That is not correct, since the scripts generates the modules for you, but there is not need to use them to run the code.

diehlpk commented 3 years ago

Also, the idea of installing all the libraries separately is not good. This requires a significant amount of resources for compiling and building huge libraries like CMAKE and VTK.

@chennachaos You can use your own cmake, however, HPX relies on a very recent one. Especially in Ubuntu the CMake version is not recent enough.

diehlpk commented 3 years ago

Also, the idea of installing all the libraries separately is not good. This requires a significant amount of resources for compiling and building huge libraries like CMAKE and VTK.

@chennachaos Note that you are using the tool chain for HPX system to build the code. If you want to use it on your desktop computer you might want to follow this instructions

https://github.com/nonlocalmodels/NLMech#compiling-library

There is no need to compile all the dependencies, like I have shown here for Fedora

https://github.com/nonlocalmodels/buildscripts/blob/main/bash/buildFedora.sh

CMake checks for installed libraries on your system or you can provide the path if the library is not installed in the system directories.

Since we have some hpx package on Fedora, there is no need to compile HPX. On other systems you have to build HPX, but you can use boost, hwloc, and most of the dependencies from the package manager.

I for example use for development the instruction on Fedora in the script above.

diehlpk commented 3 years ago

Hi @diehlpk, the other dependencies you installed using apt-get install needs to be added to the list of dependencies on the installation page.

@chennachaos That is a tricky question, because this are not dependencies of NLMech. These are dependencies of the dependencies of NLMech. So I am not sure if it is really necessary to add a full list of dependencies of the dependencies. Especially were should we stop to track down the dependencies?

Note that these dependencies are specific for the server version of Ubuntu. If I use the desktop version, there is no need to install openssl.

Since there is not a package manager, like pip or anaconda in python, I do believe it is really difficult to provide on script to install the code on various operating systems. We could provide specific scripts for each OS, but that would be too much work to keep them updated.

vijaysm commented 3 years ago

@chennachaos That is a tricky question, because this are not dependencies of NLMech. These are dependencies of the dependencies of NLMech. So I am not sure if it is really necessary to add a full list of dependencies of the dependencies. Especially were should we stop to track down the dependencies?

Unfortunately, this is the curse of dependency stacks. It is important to make clear the overall minimum set of dependency list (and yes, you inherit dependencies of your dependencies as well) that you need to build NLMech consistently. If you want to restrict usage to Fedora and Ubuntu, then fine tune the scripts for the platforms and document that installations in other platforms are untested/unverified.

I raised this issue initially, but I am a bit more convinced that there is a viable usage path with the Docker images that I have already used to verify the NLMech examples.

Note that these dependencies are specific for the server version of Ubuntu. If I use the desktop version, there is no need to install openssl.

Just to clarify, I had to install openssl on the Ubuntu desktop (?) version since I was just using the docker image for 20.04.

Since there is not a package manager, like pip or anaconda in python, I do believe it is really difficult to provide on script to install the code on various operating systems. We could provide specific scripts for each OS, but that would be too much work to keep them updated.

One suggestion here is to try to use a package manager like spack to simplify dependency builds instead of using custom written bash scripts which are prone to failure depending on the base system and available pre-installed dependencies. With spack, there are recipes you could create in order to build NLMech and TPLs consistently. That being said, I have not checked if they support HPX and other dependencies that you require already.

diehlpk commented 3 years ago

One suggestion here is to try to use a package manager like spack to simplify dependency builds instead of using custom written bash scripts which are prone to failure depending on the base system and available pre-installed dependencies. With spack, there are recipes you could create in order to build NLMech and TPLs consistently. That being said, I have not checked if they support HPX and other dependencies that you require already.

I think we have HPX available in spack and maybe in easyinstall. However, blaze and blaze_iterative are not available in spack and easytinsall. I do not have the capacity to add blaze and easyinstall to them right now. Maybe in the future, we will work on that but that will not happen in the near future.

diehlpk commented 3 years ago

@chennachaos That is a tricky question, because this are not dependencies of NLMech. These are dependencies of the dependencies of NLMech. So I am not sure if it is really necessary to add a full list of dependencies of the dependencies. Especially were should we stop to track down the dependencies?

Unfortunately, this is the curse of dependency stacks. It is important to make clear the overall minimum set of dependency list (and yes, you inherit dependencies of your dependencies as well) that you need to build NLMech consistently. If you want to restrict usage to Fedora and Ubuntu, then fine tune the scripts for the platforms and document that installations in other platforms are untested/unverified.

@vijaysm and @chennachaos I added them to the Readme.md and also listed the command to install them using apt or dnf.

In addition, I made some comment that some dependencies can be installed using the system packages. I added the example to install them using dnf or apt as well.

vijaysm commented 3 years ago

@diehlpk thanks for updating the documentation.

With regards to spack, looks like they do have blaze [1] if that was a bottleneck before.

[1] https://spack.readthedocs.io/en/latest/package_list.html#blaze

diehlpk commented 3 years ago

@diehlpk thanks for updating the documentation.

With regards to spack, looks like they do have blaze [1] if that was a bottleneck before.

[1] https://spack.readthedocs.io/en/latest/package_list.html#blaze

Ok, but we still miss Blaze_Iterative. I agree, having the code in Spack would be really nice, however, I do not have the capacity to add Blaze_Iterative to Spack and maintain the package. Maybe if the amount of user grow, I will invest time to add Blaze_iterative and NLMech to Spack.

vijaysm commented 3 years ago

Ok, but we still miss Blaze_Iterative.

Sure fair enough. But something to consider for future - and perhaps a motivation to convince the Blaze_Iterative devs to maintain the recipe as well for broader usage. Not really relevant to this review and so I don't want to diverge too much from topic.

Overall, I have preferred the Dockerized solution to getting started with NLMech, since this avoids the need for a user to build and install all these complex TPLs. You can add the build all TPLs from source as part of the developer guide or for advanced users since it would take a non-trivial amount of time to get it built, on some user machine.

My review for NLMech is almost complete. I will take a look to confirm that there are no other pending comments I have written down elsewhere. Thanks for all the quick responses @diehlpk

openjournals / joss-reviews