[REVIEW]: papaya2 & Morphometer: 2D Irreducible Minkowski Tensor computation

whedon commented 4 years ago

Submitting author: @skapfer (Sebastian Kapfer) Repository: https://github.com/morphometry/papaya2 Version: v1.0.0 Editor: @hugoledoux Reviewers: @kenohori, @rmjarvis Archive: 10.5281/zenodo.4071543

: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/8fb83decab82a0901d24a74dccd60fec"><img src="https://joss.theoj.org/papers/8fb83decab82a0901d24a74dccd60fec/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/8fb83decab82a0901d24a74dccd60fec/status.svg)](https://joss.theoj.org/papers/8fb83decab82a0901d24a74dccd60fec)

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

@kenohori, 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 @hugoledoux 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 @kenohori

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 (@skapfer) 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: Do the authors clearly state 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 @rmjarvis

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 (@skapfer) 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: Do the authors clearly state 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.
[ ] 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?

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @kenohori it looks like you're currently assigned to review this paper :tada:.

: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.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

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

@whedon commands

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

@whedon generate pdf

whedon commented 4 years ago

PDF failed to compile for issue #2538 with the following error:

Can't find any papers to compile :-(

hugoledoux commented 4 years ago

/ooo August 1 until August 16

ooo[bot] commented 4 years ago

:+1: Marked @hugoledoux as OOO from Saturday, August 1st 2020 to Sunday, August 16th 2020. :calendar:

hugoledoux commented 4 years ago

@whedon add @rmjarvis as reviewer

whedon commented 4 years ago

OK, @rmjarvis is now a reviewer

ManniDasMammut commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago

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

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

kenohori commented 4 years ago

Hello,

Started working on the review today and noticed a few things. Regarding the code:

The Makefile broadly works, but it doesn't look for paths, and so is not really an automated solution. Include and library paths need to be added manually. It doesn't link (-l) against python (for pypaya2).
The Python module is 2.x only, which was retired on January 1.
The software fails to build with CGAL 5.x (because it needs -std=c++14). IMO, it should also be updated to use the newer header-only mode of CGAL.

Regarding the documentation:

There is no README information. The link to the website works, but this is not really a good practice (not versioned, no instructions when checking out repo, can be removed anytime).
There is no clear list of dependencies. Some are listed throughout the documentation, but this is not ideal. numpy is not mentioned.
Doesn't include information about test data or testing in general.
There are no instructions for third parties to contribute to the software.

Finally, I want to know what is the link to Morphometer? Is Morphometer open source? Is it a web application only or is there some backend?

skapfer commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago

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

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

skapfer commented 4 years ago

Dear @kenohori , thank you for the constructive feedback. We have addressed your comments in the code repository and the paper.

Specifically:

we have made sure that the software builds correctly with CGAL 5 and Python 3
we have included the JavaScript binding in the repository
we have reorganized the repository to more clearly identify the components
we have added a README, including instructions on how to contribute and where to file bugs, and how to run the test suite
we have added READMEs which detail installation requirements and instructions for the language bindings

The morphometer is a pure web application (no backend) which uses a version of papaya2 compiled to JavaScript. For completeness, we have added this binding to the code repository as well. The rest of the Morphometer is merely wrapping the library in a nice webpage. Its source code is GPLv3 (see imprint).

We hope that we could address your concerns and would like to thank you again for your feedback.

Sebastian & Fabian, on behalf of the authors

rmjarvis commented 4 years ago

Some comments re installation:

I had to manually add -std=c++11 (or later). I don't think this breaks newer compilers and is required for older ones, so I'd recommend just adding that to the Makefile.
I hit an error compiling the demos because I didn't have CGAL installed. Nothing in the README says that this is required, so I'd recommend at least adding that instruction to the README file. A better solution might be to explicitly describe the different demos you can make with their corresponding make commands (make sersic, make ppanalysis, etc.) and just point out that CGAL is required for ppanalysis and CCfits is required for banana.

After installing CGAL (via conda install cgal), the compilation of ppanalysis failed:

$ make ppanalysis
c++ -g -O3 -std=c++11 -Wall -Werror -I../include -DHAVE_CGAL -c -o ppanalysis.o ppanalysis.cpp
In file included from /anaconda3/envs/py3.7/include/boost/random/detail/integer_log2.hpp:19:0,
             from /anaconda3/envs/py3.7/include/boost/random/detail/large_arithmetic.hpp:19,
             from /anaconda3/envs/py3.7/include/boost/random/detail/const_mod.hpp:23,
             from /anaconda3/envs/py3.7/include/boost/random/linear_congruential.hpp:30,
             from /anaconda3/envs/py3.7/include/boost/random/additive_combine.hpp:27,
             from /anaconda3/envs/py3.7/include/boost/random.hpp:36,
             from /anaconda3/envs/py3.7/include/CGAL/algorithm.h:39,
             from /anaconda3/envs/py3.7/include/CGAL/Hilbert_sort_base.h:26,
             from /anaconda3/envs/py3.7/include/CGAL/Hilbert_sort_median_2.h:27,
             from /anaconda3/envs/py3.7/include/CGAL/Hilbert_sort_2.h:25,
             from /anaconda3/envs/py3.7/include/CGAL/hilbert_sort.h:28,
             from /anaconda3/envs/py3.7/include/CGAL/spatial_sort.h:26,
             from /anaconda3/envs/py3.7/include/CGAL/Triangulation_2.h:45,
             from /anaconda3/envs/py3.7/include/CGAL/Delaunay_triangulation_2.h:28,
             from ../include/papaya2/voronoi.hpp:11,
             from ppanalysis.cpp:11:
/anaconda3/envs/py3.7/include/boost/pending/integer_log2.hpp:7:89: note: #pragma message: This header is deprecated. Use <boost/integer/integer_log2.hpp> instead.
BOOST_HEADER_DEPRECATED("<boost/integer/integer_log2.hpp>");
                                                                                     ^
c++ -g -O3 -std=c++11 -Wall -Werror -I../include -o ppanalysis ppanalysis.o  -lCGAL -LCGAL_Core -lgmp
ld: warning: directory not found for option '-LCGAL_Core'
$ ppanalysis
dyld: Library not loaded: @rpath/libCGAL.13.dylib
Referenced from: /Users/Mike/papaya2/demos/./ppanalysis
Reason: image not found
Abort trap: 6

Installation of banana also failed:

$ make banana
c++ -g -O3 -std=c++11 -Wall -Werror -I../include -o banana banana.cpp  -lCCfits -lcfitsio
Undefined symbols for architecture x86_64:
"CCfits::FitsException::FitsException(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool&)", referenced from:
  CCfits::Image<double>::readImage(fitsfile*, long, long, double*, std::vector<long, std::allocator<long> > const&, bool&) in ccnYZ9YS.o
  CCfits::Image<float>::readImage(fitsfile*, long, long, float*, std::vector<long, std::allocator<long> > const&, bool&) in ccnYZ9YS.o
  CCfits::Image<unsigned char>::readImage(fitsfile*, long, long, unsigned char*, std::vector<long, std::allocator<long> > const&, bool&) in ccnYZ9YS.o
  CCfits::Image<unsigned int>::readImage(fitsfile*, long, long, unsigned int*, std::vector<long, std::allocator<long> > const&, bool&) in ccnYZ9YS.o
  CCfits::Image<int>::readImage(fitsfile*, long, long, int*, std::vector<long, std::allocator<long> > const&, bool&) in ccnYZ9YS.o
  CCfits::Image<long long>::readImage(fitsfile*, long, long, long long*, std::vector<long, std::allocator<long> > const&, bool&) in ccnYZ9YS.o
  CCfits::PrimaryHDU<unsigned short>::readImage(long, long, unsigned short*) in ccnYZ9YS.o
  ...
"CCfits::FITS::FITS(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CCfits::RWmode, bool, std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&)", referenced from:
  FitsFile::FitsFile(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unsigned int) in ccnYZ9YS.o
"CCfits::FITSUtil::UnrecognizedType::UnrecognizedType(std::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool)", referenced from:
  CCfits::FITSUtil::MatchType<long long>::operator()() in ccnYZ9YS.o
"CCfits::FitsFatal::FitsFatal(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)", referenced from:
  void CCfits::PHDU::read<double>(std::valarray<double>&, long, long, double*) in ccnYZ9YS.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
make: *** [banana] Error 1

I have CCfits installed (also via conda), and compiling with c++ -v shows that it is in my library search path, and the compiler didn't give an error about not finding libCCfits.dylib, so I think it found it. But it seems to be trying to use a function that it can't find the link to.

The unit tests fail with both g++ and clang. First the g++ error: (c++ aliases to g++ on my system)


$ make
c++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o test_image.o test_image.cpp
c++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o test_tools.o test_tools.cpp
c++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o test_imt.o test_imt.cpp
c++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o runtests.o runtests.cpp
c++ -g -O3 -std=c++11 -Wall -Werror -I../include -o runtests test_image.o test_tools.o test_imt.o  runtests.o 
./runtests
dyld: lazy symbol binding failed: Symbol not found: __ZNSt6chrono3_V212system_clock3nowEv
Referenced from: /Users/Mike/papaya2/test/./runtests
Expected in: /usr/lib/libstdc++.6.0.9.dylib

dyld: Symbol not found: __ZNSt6chrono3_V212system_clock3nowEv Referenced from: /Users/Mike/papaya2/test/./runtests Expected in: /usr/lib/libstdc++.6.0.9.dylib

make: *** [default] Abort trap: 6

* The error with clang is on compiliation:

$ CXX=clang++ make clang++ -g -O3 -std=c++11 -Wall -Werror -I../include -c -o test_image.o test_image.cpp clang++ -g -O3 -std=c++11 -Wall -Werror -I../include -c -o test_tools.o test_tools.cpp test_tools.cpp:66:11: error: no matching member function for call to 'assign' v.assign(r.begin(), r.end()); ^~~~ /anaconda3/envs/py3.7/bin/../include/c++/v1/vector:611:10: note: candidate function not viable: no known conversion from 'papaya2::IntegerRange::iterator' to 'std::1::vector<int, std::1::allocator >::size_type' (aka 'unsigned long') for 1st argument void assign(size_type n, const_reference u); ^ /anaconda3/envs/py3.7/bin/../include/c++/v1/vector:599:9: note: candidate template ignored: requirement '!is_cpp17_forward_iterator::value' was not satisfied [with _InputIterator = papaya2::IntegerRange::iterator] assign(_InputIterator first, _InputIterator last); ^ /anaconda3/envs/py3.7/bin/../include/c++/v1/vector:609:9: note: candidate template ignored: requirement 'is_constructible<int, void>::value' was not satisfied [with _ForwardIterator = papaya2::IntegerRange::iterator] assign(_ForwardIterator __first, _ForwardIterator last); ^ /anaconda3/envs/py3.7/bin/../include/c++/v1/vector:615:10: note: candidate function not viable: requires single argument 'il', but 2 arguments were provided void assign(initializer_list il) ^ 1 error generated. make: *** [test_tools.o] Error 1



Now I recognize that portability is hard, and I'm guessing none of the developers are using Macs.  These errors are all on a MacOS 10.13 system.  I'd recommend using a CI such as Travis to regularly build and run the tests and demos on a variety of systems including both Mac and Linux and ideally with at least a couple compilers.  This will help you determine what instructions to give about what is really required to make it work on different systems.  Especially with respect to dependencies, since Travis will make you install all of them explicitly.

rmjarvis commented 4 years ago

Now some tests on a linux box. Specifically Scientific Linux release 7.3:

Test suite runs but fails. The failures seem to be from testing double precision equality for things that are not exactly equal at machine precision. Probably just the unit tests should be testing with some tolerance in the difference (say 1.e-12)


$ make
g++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o test_image.o test_image.cpp
g++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o test_tools.o test_tools.cpp
g++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o test_imt.o test_imt.cpp
g++ -g -O3 -std=c++11 -Wall -Werror -I../include   -c -o runtests.o runtests.cpp
g++ -g -O3 -std=c++11 -Wall -Werror -I../include -o runtests test_image.o test_tools.o test_imt.o  runtests.o 
./runtests


runtests is a Catch v2.2.3 host application.
Run with -? for options

-------------------------------------------------------------------------------
marching squares algorithm
  negating should flip the normals
  config #12
-------------------------------------------------------------------------------
test_image.cpp:164
...............................................................................

test_image.cpp:169: FAILED:
  CHECK( i_imt.imt(3) == -b_imt.imt(3) )
with expansion:
  (-0.962434,0.288447)
  ==
  (-0.962434,0.288447)

-------------------------------------------------------------------------------
marching squares algorithm
  negating should flip the normals
  config #12, connect black
-------------------------------------------------------------------------------
test_image.cpp:172
...............................................................................

test_image.cpp:178: FAILED:
  CHECK( i_imt.imt(3) == -b_imt.imt(3) )
with expansion:
  (-0.962434,0.288447)
  ==
  (-0.962434,0.288447)

-------------------------------------------------------------------------------
marching squares algorithm
  negating should flip the normals
  critical config #6
-------------------------------------------------------------------------------
test_image.cpp:185
...............................................................................

test_image.cpp:190: FAILED:
  CHECK( i_imt.imt(3) == -b_imt.imt(3) )
with expansion:
  (-0.461011,-0.730043)
  ==
  (-0.461011,-0.730043)

-------------------------------------------------------------------------------
marching squares algorithm
  negating should flip the normals
  critical config #6, connect white
-------------------------------------------------------------------------------
test_image.cpp:193
...............................................................................

test_image.cpp:199: FAILED:
  CHECK( i_imt.imt(3) == -b_imt.imt(3) )
with expansion:
  (-0.108522,0.532048)
  ==
  (-0.108522,0.532048)

-------------------------------------------------------------------------------
marching squares algorithm
  negating should flip the normals
  critical config #9
-------------------------------------------------------------------------------
test_image.cpp:206
...............................................................................

test_image.cpp:211: FAILED:
  CHECK( i_imt.imt(3) == -b_imt.imt(3) )
with expansion:
  (0.108522,-0.532048)
  ==
  (0.108522,-0.532048)

-------------------------------------------------------------------------------
marching squares algorithm
  negating should flip the normals
  critical config #9, connect white
-------------------------------------------------------------------------------
test_image.cpp:214
...............................................................................

test_image.cpp:220: FAILED:
  CHECK( i_imt.imt(3) == -b_imt.imt(3) )
with expansion:
  (0.461011,0.730043) == (0.461011,0.730043)

===============================================================================
test cases:   8 |   7 passed | 1 failed
assertions: 250 | 244 passed | 6 failed

make: *** [default] Error 6
```
* The demos sersic, banana, and imganalysis compiled fine after adding `-std=c++11` and manually adding the appropriate `-I` and `-L` locations for CCfits (and CGAL).
* `ppanalysis` failed on this system, since the compiler (gcc 4.8.5) doesn't support c++-14, and it seems that that latest CGAL requires it.  The errors included the following:
```
/gpfs/mnt/gpfs02/astro/workarea/mjarvis/.conda/envs/py3.7/include/CGAL/iterator.h:663:11: error: ‘decay_t’ in namespace ‘std’ does not name a type
   typedef std::decay_t<typename cpp11::result_of<Op(arg_type)>::type> value_type;
           ^
/gpfs/mnt/gpfs02/astro/workarea/mjarvis/.conda/envs/py3.7/include/CGAL/iterator.h:665:11: error: ‘value_type’ does not name a type
   typedef value_type const*                                     pointer;
           ^
/gpfs/mnt/gpfs02/astro/workarea/mjarvis/.conda/envs/py3.7/include/CGAL/iterator.h:1321:58: error: expected ‘,’ or ‘...’ before ‘<’ token
 auto to_tuple(std::tuple<Args...> &t, std::index_sequence<Is...>)
                                                          ^
/gpfs/mnt/gpfs02/astro/workarea/mjarvis/.conda/envs/py3.7/include/CGAL/iterator.h:1321:65: error: ‘to_tuple’ function uses ‘auto’ type specifier without trailing return type [-Werror]
 auto to_tuple(std::tuple<Args...> &t, std::index_sequence<Is...>)
/gpfs/mnt/gpfs02/astro/workarea/mjarvis/.conda/envs/py3.7/include/CGAL/Kernel/hash_functions.h:25:8 error: ‘enable_if_t’ in namespace ‘std’ does not name a type
 inline std::enable_if_t<std::is_same<typename K::Rep_tag, Cartesian_tag>::value, std::size_t>
        ^
```
I think these are all related to c++-14 features.  I don't have a c++-14 compiler on that system, so I couldn't test that demo further there.

rmjarvis commented 4 years ago

Onto a third system now: SUSE Linux Enterprise Server 15, which has gcc 8.3.0.

The unit tests compiled and ran successfully.
I had to add the right -I and -L locations manually to tell it where CCfits and CGAL were installed. But then banana, sersic and imganalysis all built successfully.

ppanalysis compiled successfully, but failed the linking step:

$ make ppanalysis
g++ -g -O3 -Wall -Werror -I../include -I/global/u2/m/mjarvis/.conda/envs/my_root/include -L/global/u2/m/mjarvis/.conda/envs/my_root/lib -DHAVE_CGAL -c -o ppanalysis.o ppanalysis.cpp
g++ -g -O3 -Wall -Werror -I../include -I/global/u2/m/mjarvis/.conda/envs/my_root/include -L/global/u2/m/mjarvis/.conda/envs/my_root/lib -o ppanalysis ppanalysis.o  -lCGAL -LCGAL_Core -lgmp
/usr/bin/ld: cannot find -lCGAL
collect2: error: ld returned 1 exit status
make: *** [Makefile:48: ppanalysis] Error 1

I see a number of libraries that start with CGAL, but not one that is simple libCGAL.so:

$ ls /global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL*
/global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL_AABB_tree_cpp.so*
/global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL_Alpha_shape_2_cpp.so*
/global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL_Kernel_cpp.so*
/global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL_Mesh_3_cpp.so*
/global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL_Surface_mesher_cpp.so*
/global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL_Triangulation_2_cpp.so*
/global/u2/m/mjarvis/.conda/envs/my_root/lib/libCGAL_Triangulation_3_cpp.so*

I don't know whether this is an error in the linking step of your code or in the conda installation of CGAL. It seems plausible that there should be a single library to link to that encompasses all of these, which would logically be called libCGAL.so. But it doesn't seem to be installed along with the rest of these more specific libraries.

rmjarvis commented 4 years ago

Some high-level comments, mostly about the paper and documentation:

You piqued my interest about the utility of this package with some statements about how it provides a richer description of the shape or geometry of objects than say the 2d inertia. I work in gravitational lensing, so we regularly use 2d moments to describe the shape (and indirectly the shear) of galaxies. It works well for weak lensing, but it's not great for medium to strong lensing. So I was most attracted to the example you have of a strong lensing system. However, I couldn't really figure out how I would use it to either detect or characterize strong lensing images. The example has 4 numbers that get computed as a function of a threshold: area, perimeter, area fraction, and q_2. The first three don't seem particularly useful, although possibly some combination like perimeter^2/area might be diagnostic. q_2 seemed potentially interesting, but I couldn't find much discussion about it. It sounds a bit like ellipticity (responding to the quadrupole moment). Is it better in some way than standard ellipticities? What intuition should I develop about it? Or for the higher order Minkowski structure metrics? I feel like the paper would be significantly helped by an example like this (not necessarily this one, but probably something connected to one of your demo examples would be fine) a bit more to show how it could be useful to a real science case.
The paper includes the example of an anisotropic Gaussian random field from Morphometer, but again, there are just a handful of numbers produced for it without much explanation of what they might be good for. So it's hard for me as someone not already steeped in the use of these statistics to know why I should care. If you at least sketched out an overview of what kind of science you can do with this (say with maps of large scale structure or some material science data maybe), it would help make it a bit more concrete and potentially give me more of a clue why I might want to use it for something I'm working on.
The paper has a link to this page: https://morphometry.org/theory/anisotropy-analysis-by-imt/. This page feels to me like information that should be in the paper. This was where I learned the most about what the numbers calculated by the software were really about. It's fine for it to live on the web in addition to the paper, but I don't think a link to this page from the paper is sufficient.
It's probably too late at this point, but it wasn't obvious to me why this rewrite of papaya deserved to be a brand new package rather than just papaya version 2.0. Refactoring of code happens all the time, and it usually doesn't merit making a new package with a new name. This is normally done with release versions, even when the code has been completely redone.
I think the demos would benefit from including some example input files. Perhaps the same files that are used in the online demo. Then I could try them out immediately, look at the input and output files, mess around with some of the parameters to see how things change. Having to supply my own input (e.g. FITS image) files is a pretty big barrier to this being a useful demo. If I have a problem, I don't know whether the issue is with my input file or how I'm running it or what. But with a sample input file, you can give explicit instructions on how you can run it and what you should see as output.

skapfer commented 4 years ago

Wow, thank you for the huge amount of feedback, Mike! First of all I'd like to point out that I've never received so valuable and extensive feedback in peer review. Thank you for that!

About C++11: This is quite tough news. We just removed -std=c++11 from the Makefile because it broke CGAL 5 support, which was requested by the other referee, Ken. C++ on Unixes does seem to be moving at a quick pace these days.

Thank you for pointing out the precision problem in the test cases. Even though we cannot reproduce this on our machines, your analysis is most certainly correct and we think we fixed this on master branch.

On your Mac, the clang version seems to be very recent and seems to worry about our iterator class (__is_cpp17_forward_iterator sounds odd in a C++11 compilation though - we'll look into this here https://github.com/morphometry/papaya2/issues/14).

For some reason, CGAL 5 snuck onto two of your systems even though your compiler does not support it (yet). CGAL 5 requires working C++14 support (decay_t and friends). Papaya2 works with older CGAL releases though, which do work on older compilers (personally I use CGAL 4.9 with GCC 6.3.0 most of the time) We will add explicit installation instructions for CGAL 4.x vs. 5.0, and a list of dependencies for each demo, as you suggested.

BTW: libstdc++.6.0.9 (which your Mac gcc seems to use) doesn't have full C++ 11 support yet. I think that's why the test suite fails to compile with that one (the __ZNSt6chrono3_V212system_clock3nowEv symbol is part of the C++11 standard).

It is a good suggestion to include some input files for the demo programs and interpretation. We'll add that.

We are uncertain about the scope of the paper. Certainly we could not only reference https://morphometry.org/theory/anisotropy-analysis-by-imt/ and other papers by us and coworkers but also include more of that information in the paper. However, that would inflate the length of the paper a lot. The JOSS guidelines to authors suggest a length of 250-1000 words for the paper, and we're rather at the upper end of the scale already.

We will be working on resolving the technical problems in the next few days. We would appreciate guidance on whether we should expand the contents of the paper, and if yes, to what detail (probably mathematical derivations would go too far)?

rmjarvis commented 4 years ago

The JOSS guidelines to authors suggest a length of 250-1000 words for the paper, and we're rather at the upper end of the scale already.

Ah, I didn't realize this. I'm used to reading significantly longer papers, so this paper felt very short to me. But if that's the case, probably my suggestions about increasing the scope of the paper are off base. Maybe just some quick overviews of some of the ways you can use the code for science then, with pointers to the relevant papers that use these techniques for people to read up on further. I think that would be helpful to motivate interest in the various ways one can use the software.

hugoledoux commented 4 years ago

Indeed, we try to keep the paper <1000words. The software and the docs are the main results for JOSS.

That being said, you could add some results and comparison to the docs, if you think it is appropriate.

hugoledoux commented 4 years ago

For some reason, CGAL 5 snuck onto two of your systems even though your compiler does not support it (yet). CGAL 5 requires working C++14 support (decay_t and friends). Papaya2 works with older CGAL releases though, which do work on older compilers (personally I use CGAL 4.9 with GCC 6.3.0 most of the time) We will add explicit installation instructions for CGAL 4.x vs. 5.0, and a list of dependencies for each demo, as you suggested.

This sounds good. There was a discussion among JOSS editors whether software should use the latest version libraries, and yes we'd prefer that very much. The discussion was whether Python2.7 should be the only Python supported, and the answer was a clear no, Python3 ought to be supported. For CGAL, v5 would be nice, but indeed I can imagine that 4.x is still used if compilers are older.

skapfer commented 4 years ago

We would like to thank @kenohori and @rmjarvis for their thorough review and helpful suggestions!

We have rewritten the build system in order to work with CGAL 4.x and 5, Python 2.x and 3.x, various C++ compilers and added CI workflows for Linux and MacOS platforms. The Makefile will now also detect the CGAL version in use. We think we have corrected all the bugs found by @rmjarvis.

In addition, we extended the README files to include installation instructions and complete lists of external dependencies.

In the paper, we have extended the examples by adding interpretation of the morphometric data displayed in the analysis. We have swapped out two of the examples in favor of more intuitive ones which better convey the gist of the analysis.

The new version has an extensive list of references with various applications, and we clarified the language in some places.

We hope that with the latest revision we have addressed all reviewers' concerns and answered all open questions.

skapfer commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago

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

whedon commented 4 years ago

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

hugoledoux commented 4 years ago

This sounds great @skapfer.

When @kenohori and @rmjarvis have time, if they could go over the update and confirm (clicking the checkboxes above) or not, then we can move forward.

kenohori commented 4 years ago

Thank you for all the work, @skapfer! It's all looking quite well.

Honestly, not being very acquainted with Minkowski tensors, I don't have much to add regarding the paper. It's clear, reads well and serves as a very good introduction to the topic. The examples are very nice and representative. They also piqued my interest about using Minkowski tensors as shape descriptors. I think the paper has all the elements required in JOSS.

Regarding the code, I managed to get everything working on Mac (macOS 11 beta) and Linux (Ubuntu 20.04) with recent versions of clang and gcc. I only had to do minor changes to some Makefiles:

add include paths (-I) for CGAL 5, libraries too (-L) for CGAL 4
remove -Werror, since boost causes many warnings when building any CGAL code using clang, which then causes the CGAL checks to fail

For the first item, IMO the most sustainable build option would be to use something like CMake to find the required paths, but I'd settle with instructions to add your paths in the README. For the second, I think you should just remove -Werror in the Makefile since clang is notoriously picky and boost isn't getting fixed anytime soon.

Finally, although not strictly related to papaya2, I think it's important to add morphometer itself to the repository (or its own repository).

ManniDasMammut commented 4 years ago

Dear @kenohori,

thank you very much for your feedback.

Regarding the morphometer, in our first iteration of the paper, we already have refined the title and the scope of the paper to only describe the library papaya2. We realized that having both, papaya2 and the morphometer, in the paper is too much and not very comprehensible.

In the latest iteration of the paper, we only use the morphometer to create the images and histograms, but this could have been done as well with our demo programs and any other plotting tool. We are currently thinking about how to make morphometer code available. Describing the whole morphometer and all its functions is definitely something we want to publish on its own, as we realized that this is too much for this paper.

We extended the "Installing dependencies" section in the README.md in the demos folder, to explain how to add libraries which are not in the standard paths and instructions how to manually download CGAL.

We have removed -Werror from the Makefile as it seems to cause issues with the latest macOS beta. Nevertheless, everything runs smoothly on the latest stable macOS (Catalina).

We hope that the installation now works for all commonly used Unix platforms.

kenohori commented 4 years ago

Thanks for the response, @ManniDasMammut.

I do agree that morphometer should be in its own repository and publication. Just wanted to point out that it's currently rather user unfriendly to download it (by finding the required files manually).

Regarding macOS Catalina, I have the same issues with -Werror. My best guess is that it's an issue with clang 10.

ManniDasMammut commented 4 years ago

Dear @kenohori, can you please provide an error message, because we cannot reproduce the error on our systems with clang 10 (Catalina and High Sierra). Thank you very much.

kenohori commented 4 years ago

Sure! This is what happens when I run the cgal checks manually:

demos git:(master) ✗ c++ -g -O3 -std=c++14 -Wall -Werror -I../include -I/usr/local/include ../config/test-recent-cgal.cpp
In file included from ../config/test-recent-cgal.cpp:1:
In file included from /usr/local/include/CGAL/Polygonal_surface_reconstruction.h:18:
In file included from /usr/local/include/CGAL/internal/hypothesis.h:18:
In file included from /usr/local/include/CGAL/convex_hull_2.h:21:
In file included from /usr/local/include/CGAL/ch_akl_toussaint.h:49:
In file included from /usr/local/include/CGAL/Convex_hull_2/ch_akl_toussaint_impl.h:20:
In file included from /usr/local/include/CGAL/convexity_check_2.h:123:
In file included from /usr/local/include/CGAL/Convex_hull_2/convexity_check_2_impl.h:21:
/usr/local/include/boost/bind.hpp:36:1: warning: The practice of declaring the
      Bind placeholders (_1, _2, ...) in the global namespace is deprecated.
      Please use <boost/bind/bind.hpp> + using namespace boost::placeholders, or
      define BOOST_BIND_GLOBAL_PLACEHOLDERS to retain the current behavior.
      [-W#pragma-messages]
BOOST_PRAGMA_MESSAGE(
^
/usr/local/include/boost/config/pragma_message.hpp:24:34: note: expanded from
      macro 'BOOST_PRAGMA_MESSAGE'
# define BOOST_PRAGMA_MESSAGE(x) _Pragma(BOOST_STRINGIZE(message(x)))
                                 ^
<scratch space>:92:2: note: expanded from here
 message("The practice of declaring the Bind placeholders (_1, _2, ...) ...
 ^
In file included from ../config/test-recent-cgal.cpp:1:
In file included from /usr/local/include/CGAL/Polygonal_surface_reconstruction.h:18:
In file included from /usr/local/include/CGAL/internal/hypothesis.h:16:
In file included from /usr/local/include/CGAL/Surface_mesh.h:18:
In file included from /usr/local/include/CGAL/Surface_mesh/Surface_mesh.h:42:
In file included from /usr/local/include/CGAL/boost/graph/copy_face_graph.h:20:
In file included from /usr/local/include/CGAL/boost/graph/Euler_operations.h:21:
In file included from /usr/local/include/CGAL/boost/graph/helpers.h:975:
In file included from /usr/local/include/CGAL/boost/graph/generators.h:19:
In file included from /usr/local/include/CGAL/Random.h:33:
/usr/local/include/boost/random/linear_congruential.hpp:140:20: error: 
      overlapping comparisons always evaluate to false
      [-Werror,-Wtautological-overlap-compare]
        if(_x <= 0 && _x != 0) {
           ~~~~~~~~^~~~~~~~~~
/usr/local/include/boost/random/linear_congruential.hpp:393:11: note: in
      instantiation of member function
      'boost::random::linear_congruential_engine<unsigned long long,
      25214903917, 11, 281474976710656>::seed' requested here
    { lcf.seed(cnv(x0)); }
          ^
1 warning and 1 error generated.

skapfer commented 4 years ago

@kenohori This should not be happening with the most version of the demos. Can you try that after git pull?

kenohori commented 4 years ago

Indeed it doesn't happen with the most recent version of the demos (without -Werror).

Maybe it was a misunderstanding? I thought you wanted me to provide the boost warnings that cause the clang checks to fail when -Werror is present.

ManniDasMammut commented 4 years ago

@kenohori, yes, this was a misunderstanding. So everything should be fine now?

kenohori commented 4 years ago

Yes, everything is working now!

jennywagner commented 4 years ago

@rmjarvis Are you happy with the changes we made or is there anything left to do until the work can be published?

rmjarvis commented 4 years ago

I like the changes to the paper. The new examples gave me a much better feel for the potential uses of the software. And I'm satisfied with the updates to the documentation and examples.
On my system, make banana still had trouble with CCfits, but that's probably some kind of versioning issue, since I used conda rather than brew -- I'm happy to assume that the recommended brew installation would work fine. So I'm pleased to recommend this for publication.

hugoledoux commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago

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

whedon commented 4 years ago

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

hugoledoux commented 4 years ago

@whedon check references

hugoledoux commented 4 years ago

@whedon check references from branch joss

whedon commented 4 years ago

Attempting to check references... from custom branch joss

hugoledoux commented 4 years ago

@whedon check references

hugoledoux commented 4 years ago

Aaaannd we're reaching the end of this review, many thanks @kenohori and @rmjarvis for the very detailed review and tips and tests, I think this review shows how JOSS helps improve code/docs.

@skapfer I am happy to accept the paper, but there are a few things to fix before

First I made pull request for a few cosmetic changes: https://github.com/morphometry/papaya2/pull/19 . The references automatic checker doesn't seem to be working, would @danielskatz know why? (sorry, not sure where I can ask for this)

Second, about the title, should "Irreducible" be capitalised? I would say no, or only if "computation" is too. Your choice, JOSS has both all words capitalised, and none (well except "Minkowski Tensor" since always capitalised in the paper).

Third, when the above is fixed, could you:

[ ] Make a tagged release of your software, and list the version tag of the archived version here.
[x] Archive the reviewed software in Zenodo or a similar service (e.g., figshare, an institutional repository)
[x] Check the archival deposit (e.g., in Zenodo) has the correct metadata. This includes the title (should match the paper title) and author list (make sure the list is correct and people who only made a small fix are not on it). You may also add the authors' ORCID.
[x] Please list the DOI of the archived version here.

I can then move forward with accepting the submission.

skapfer commented 4 years ago

We're excited to see this move forward :partying_face:

I've archived the current state of things (as of the end of the review process) aka git tag 2020-10-07 on Zenodo with DOI

10.5281/zenodo.4071544

It now lives here: https://zenodo.org/record/4071544

We're thinking of 'Irreducible Minkowski Tensors' (IMTs for short) as a proper name, which is why we titlecased it. If that is not possible for some reason, we prefer titlecasing the whole title, i.e. "papaya2: 2D Irreducible Minkowski Tensor Computation".

I've checked the author list and title on Zenodo agree with the paper.

Cheers,

Sebastian

skapfer commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago

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

whedon commented 4 years ago

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

openjournals / joss-reviews