openjournals / joss-reviews

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

[REVIEW]: Castor: A C++ library to code "à la matlab" #3965

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@matthieuaussal<!--end-author-handle-- (Matthieu Aussal) Repository: https://github.com/leprojetcastor/castor Branch with paper.md (empty if default branch): Version: 1.0 Editor: !--editor-->@jbytecode<!--end-editor-- Reviewers: @mkitti, @pitsianis Archive: 10.5281/zenodo.6360120

: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

Status badge code:

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

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

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

  1. Make sure you're logged in to your GitHub account
  2. 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 @jbytecode 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 @mkitti

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @pitsianis

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @mkitti, @junliu2050 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:

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

watching

  1. 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 2 years ago

Wordcount for paper.md is 2074

whedon commented 2 years ago
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.19 s (421.7 files/s, 219330.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                     7           3977           6283          22256
C++                             12            385            365           2080
reStructuredText                46           2015           2742           1332
Markdown                         2             63              0            239
CMake                            8             40             19            149
TeX                              1              9              0             58
YAML                             2              9              8             46
Python                           1              4              1             15
CSS                              1              2              0             11
make                             1              4              7             10
-------------------------------------------------------------------------------
SUM:                            81           6508           9425          26196
-------------------------------------------------------------------------------

Statistical information for the repository '0a6dc246b7c70f17d676860f' was
gathered on 2021/12/01.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
@marc.bakry                      1             8              8            0.04
Antoine RIDEAU                   1             1              1            0.00
Marc                             5           140              7            0.33
Matthieu Aussal                 33          4056           1918           13.29
Series Laurent                   5         35796           2680           85.58
U-INTRA\MB264355                 1           180              0            0.40
U-INTRA\mb264355                 7           135             31            0.37

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
@marc.bakry                   6           75.0         12.7              100.00
Antoine RIDEAU                1          100.0          4.8                0.00
Marc                        105           75.0         10.2                9.52
Matthieu Aussal            2667           65.8          9.6               27.78
Series Laurent            32503           90.8          0.5               18.13
U-INTRA\mb264355             84           62.2         11.9               23.81
whedon 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:

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

OK DOIs

- None

MISSING DOIs

- 10.1145/2049662.2049663 may be a valid DOI for title: The University of Florida sparse matrix collection
- 10.1109/icme.2012.93 may be a valid DOI for title: Creating the Sydney York morphological and acoustic recordings of ears database
- 10.1109/38.865875 may be a valid DOI for title: Visualizing with VTK: a tutorial

INVALID DOIs

- None
jbytecode commented 2 years ago

Dear @mkitti and @junliu2050

Thank you very much for accepting our invitation.

jbytecode commented 2 years ago

Dear @matthieuaussal

I have sent a pull request. It includes the missing DOIs that whedon suggests. Please apply the patch if you are agreed with me.

thank you in advance.

jbytecode commented 2 years ago

@whedon check references

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

OK DOIs

- 10.1145/2049662.2049663 is OK
- 10.1109/icme.2012.93 is OK
- 10.1109/38.865875 is OK

MISSING DOIs

- None

INVALID DOIs

- None
jbytecode commented 2 years ago

@whedon generate pdf

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

matthieuaussal commented 2 years ago

@whedon https://github.com/whedon generate pdf

whedon commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
matthieuaussal commented 2 years ago

@whedon generate pdf

Le 1 déc. 2021 à 13:09, whedon @.***> a écrit :

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3965#issuecomment-983579555, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHFHRGAETE5D5GCINFFJVB3UOYF6HANCNFSM5JDT4MGQ. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

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

junliu2050 commented 2 years ago

Hi, after carefully reading through the paper, I feel I may not be very suitable for reviewing such a library since I do not use or develop a similar package by myself. Hence you may assign Rodrigo Portugal as 2nd reviewer. Overall it is an interesting package, but the examples and comparison with similar packages can be improved.

jbytecode commented 2 years ago

@rosoport do you still want to review this submission?

jbytecode commented 2 years ago

@whedon remove @junliu2050 as reviewer

whedon commented 2 years ago

OK, @junliu2050 is no longer a reviewer

jbytecode commented 2 years ago

@whedon add @rosoport as reviewer

whedon commented 2 years ago

OK, @rosoport is now a reviewer

jbytecode commented 2 years ago

@whedon generate pdf

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

jbytecode commented 2 years ago

@whedon re-invite @rosoport as reviewer

whedon commented 2 years ago

The reviewer already has a pending invite.

@rosoport please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

jbytecode commented 2 years ago

@rosoport - This is the last call. I have not received a response from you either here or by e-mail. Please accept the invitation, if appropriate, by Saturday, December 11th. Otherwise I will have to find a new reviewer. For your information, best.

mkitti commented 2 years ago

The authors present, Castor, a header-only C++ template library that emulates the high-level semantics of Matlab, a proprietary commerical product licensed by Mathworks, Inc (Natick, MA). This includes several classes to represent dense, sparse, and hierarchical matrices. They combine this with operators and functions to manipulate these matrices and perform linear algebra operations. This combination of the header library with matrix manipulation and linear algebra appears to be the primary contribution. Additionally, the authors have added a graphics subsystem based on VTK to help visualize the results of the matrix operations.

mkitti commented 2 years ago

Remarks on Summary, Statement of Need, and State of the Field

An important task for the paper and in the checklist is to establish who the audience is. As written, the paper seem mainly oriented at the MATLAB user who perhaps would like to port their code to C++. This would perhaps be motivated by seeking any of restrictions of the proprietary licensing of MATLAB.

There are several alternatives to MATLAB based licensing including using Octave, NumPy, or Julia. There are all available under open source licenses, provide matrix manipulation libraries, and provide methods for visualizing the results. The summary mentions "Python", however, I think it would be worthwhile to specifically mention NumPy since that is the package in Python necessary for general matrix manipulation and linear algebra.

The critique of MATLAB, Python, and Julia made by the authors is that these are interpreted languages. In the case of MATLAB, there is increasing of use a just-in-time (JIT) compiler via the MATLAB Execution Engine in R2015b. Python does have some compiler frameworks available via Numba (via LLVM) or PyPy (JIT).

Julia is misidentified here as an interpreted language since the normal mode of executing Julia is to compile it to native code via LLVM, at first execution without a discrete compilation step. While a Julia interpreter does exist, it is mostly limited to use during debugging. Furthermore, compilation is an important factor in the use of Julia since it contributes considerable compilation latency during initial execution.

This line of critique also ignores the possibility of using a C++ interpreter in conjunction with this library such as Cling, a part of ROOT.. This would permit a more complete MATLAB-like experience particularly in offering a read-evaluate-print-loop (REPL) interface. Another her possibility is Cxx.jl, a Julia package that includes a C++ interpreter. https://compiler-research.org/assets/presentations/O_Schulz-Julia_ROOT.pdf https://compiler-research.org/assets/presentations/K_Fischer_Cxx_jl.pdf

Another potential audience for this work is the C++ developer who would like perform tasks similar to MATLAB. In this space, we need to consider other C++ matrix manipulation and linear algebra libraries such as Eigen. A direct comparison to Eigen in this paper would be helpful since it would help a C++ programmer decide between the two C++ template libraries.

Castor is an open-source C++ matrix manipulation library that implements a MATLAB-like API by using similar function names as MATLAB. In total, this perhaps could differentiate this package from other matrix manipulation languages.

To summarize, there are several potential audiences for this work:

  1. The MATLAB user who would like to port code to C++
  2. The open source developer would like to perform matrix-manipulation through ahead-of-time compilation.
  3. The C++ developer would like to matrix-manipulation similar in fashion to MATLAB

The summary and statement of need primarily focuses on audience #1, the MATLAB-user. The authors argue that an IDE greatly simplifies the use of C++. However, I believe there are many other important considerations such as memory management, garbage collection, and programming background which would need to be considered for a MATLAB-user to start using C++. An IDE alone does not eliminate the need to learn a new language and memory management concepts. This potential audience is likely limited and size and scope.

The authors should address audiences #2 and #3 directly. This library is immediately usable to someone who already has a C++ development environment, especially if they are also a MATLAB user or work with MATLAB users. For this reason, I think a comparison with other C++ matrix manipulation libraries such as Eigen is needed here. While Eigen has a distinct API from MATLAB, the APIs of NumPy and Julia are also distinct. Yet these libraries have existing and expanding userbases.

The authors should clarify the above points in the Summary and Statement of Need section. They should also add a clearly identifiable "State of the Field" section where they more clearly discuss the relationship of the software to MATLAB, other open source matrix manipulation libraries, and other C++ matrix manipulation libraries such as Eigen.

Revisions Checklist

Summary

Statement of Need

State of the Field

jbytecode commented 2 years ago

@mkitti - thank you for this comprehensive review. It is a good first round.

@matthieuaussal ‐ please follow the comments carefully and apply them, if you are not agree with the reviewer please discuss here why and why not.

matthieuaussal commented 2 years ago

@mkitti Many thanks for your review. We push an update which takes into account your remarks:

Best regards

Matthieu, Laurent and Marc

matthieuaussal commented 2 years ago

@whedon generate pdf

Le 9 déc. 2021 à 13:55, Mehmet Hakan Satman @.***> a écrit :

@mkitti https://github.com/mkitti - thank you for this comprehensive review. It is a good first round.

@matthieuaussal https://github.com/matthieuaussal ‐ please follow the comments carefully and apply them, if you are not agree with the reviewer please discuss here why and why not.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3965#issuecomment-989826338, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHFHRGA55DLSWXILC2XLVVTUQCRK5ANCNFSM5JDT4MGQ. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

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

mkitti commented 2 years ago

Thank you for the revisions.

The summary is much clearer now, and it fully encompasses the scope of the software.

For the statement of the need, discuss in general why the MATLAB semantics you do implement and why those are important and perhaps sufficient because certainly you do not implement all of MATLAB's semantics. For example, as far as I can tell, you do not have the backslash operator which Cleve Moler, the creator of MATLAB, states

The backslash operator has come to represent both the matrix origins and the usability of MATLAB.

As far as I know, \ is not a valid operator in C++. Perhaps you could implement a function mldivide. That is far from trivial (see the flowchart), but it would really emulate much of the MATLAB experience rather than just being BLAS wrapper that looks similar to MATLAB.

State of the Field

I just want to emphasize this is a required section that is missing. Part of the checklists asks "Do the authors describe how this software compares to other commonly-used packages?". Some kind of comparison is needed here, and perhaps that comparison would involve explaining why the semantics of these packages differ from MATLAB semantics.

For the transpose example you gave, I can do this with NumPy:

In [1]: import numpy as np

In [2]: from numpy import transpose

In [3]: A = np.random.randint(1,10, (5,5))

In [4]: transpose(A)
Out[4]:
array([[6, 4, 4, 2, 1],
       [4, 8, 5, 3, 4],
       [4, 9, 6, 8, 4],
       [4, 6, 3, 1, 3],
       [2, 2, 7, 4, 4]])

I would argue that the typical semantics for that operation in MATLAB would be A.' which is not valid syntax in Castor/C++ or NumPy/Python. There are other comparisons such as in this cheatsheet.

Similarly, in Julia there is also a transpose and a tick operator:

julia> transpose(A)
5×5 transpose(::Matrix{Int64}) with eltype Int64:
 5  7  4  9  9
 1  7  9  3  4
 2  7  4  4  5
 5  1  7  3  3
 5  5  9  2  2

julia> A'
5×5 adjoint(::Matrix{Int64}) with eltype Int64:
 5  7  4  9  9
 1  7  9  3  4
 2  7  4  4  5
 5  1  7  3  3
 5  5  9  2  2

The above are actually lazy operations, but work similarly to their eager implementations. We can also show that in Julia, there are also similar eager operations which are compiled.

Native code of Julia's `LinearAlgebra.transpose_f!` ```julia julia> @code_native LinearAlgebra.transpose_f!(transpose, B, A) .text ; ┌ @ transpose.jl:95 within `transpose_f!` pushq %rbp movq %rsp, %rbp pushq %r15 pushq %r14 pushq %r13 pushq %r12 pushq %rsi pushq %rdi pushq %rbx subq $168, %rsp movq %rdx, %rbx vxorps %xmm0, %xmm0, %xmm0 vmovaps %xmm0, -128(%rbp) movq $0, -112(%rbp) movq %rbx, -168(%rbp) movl $jl_get_pgcstack, %eax callq *%rax movq $4, -128(%rbp) movq (%rax), %rcx movq %rcx, -120(%rbp) leaq -128(%rbp), %rcx movq %rcx, (%rax) movq 8(%rbx), %rcx movq 16(%rbx), %rdx ; │ @ transpose.jl:96 within `transpose_f!` ; │┌ @ abstractarray.jl:95 within `axes` ; ││┌ @ array.jl:152 within `size` movq 24(%rdx), %r8 movq 32(%rdx), %r9 ; │└└ ; │ @ transpose.jl:97 within `transpose_f!` ; │┌ @ abstractarray.jl:74 within `axes` @ abstractarray.jl:95 ; ││┌ @ array.jl:152 within `size` movq 24(%rcx), %r12 ; │└└ ; │┌ @ range.jl:1043 within `==` cmpq $1, %r12 je L116 testq %r12, %r12 jne L124 ; ││┌ @ range.jl:609 within `isempty` ; │││┌ @ operators.jl:378 within `>` ; ││││┌ @ int.jl:83 within `<` testq %r9, %r9 ; │└└└└ je L129 jmp L153 ; │┌ @ range.jl:1044 within `==` ; ││┌ @ range.jl:1057 within `_has_length_one` ; │││┌ @ promotion.jl:468 within `==` L116: cmpq $1, %r9 ; │└└└ je L129 jmp L153 ; │┌ @ range.jl:1045 within `==` @ promotion.jl:468 L124: cmpq %r9, %r12 ; │└ jne L153 ; │┌ @ abstractarray.jl:74 within `axes` @ abstractarray.jl:95 ; ││┌ @ array.jl:152 within `size` L129: movq 32(%rcx), %rbx ; │└└ ; │┌ @ range.jl:1043 within `==` cmpq $1, %rbx je L230 testq %rbx, %rbx jne L752 ; ││┌ @ range.jl:609 within `isempty` ; │││┌ @ operators.jl:378 within `>` ; ││││┌ @ int.jl:83 within `<` testq %r8, %r8 ; │└└└└ je L236 L153: movq $316687360, -96(%rbp) # imm = 0x12E04400 movabsq $jl_apply_generic, %rdi leaq -96(%rbp), %rsi movl $291737952, %ecx # imm = 0x11639160 movq %rsi, %rdx movl $1, %r8d callq *%rdi movq %rax, -112(%rbp) movq %rax, -96(%rbp) movl $351419696, %ecx # imm = 0x14F23D30 movq %rsi, %rdx movl $1, %r8d callq *%rdi movabsq $jl_throw, %rdx movq %rax, %rcx callq *%rdx ; │┌ @ range.jl:1044 within `==` ; ││┌ @ range.jl:1057 within `_has_length_one` ; │││┌ @ promotion.jl:468 within `==` L230: cmpq $1, %r8 ; │└└└ jne L153 ; │ @ transpose.jl within `transpose_f!` L236: movq %rax, -136(%rbp) ; │ @ transpose.jl:100 within `transpose_f!` ; │┌ @ int.jl:88 within `*` movq %r9, %rax imulq %r8, %rax ; │└ ; │┌ @ int.jl:477 within `<=` cmpq $256, %rax # imm = 0x100 movq %rcx, -72(%rbp) ; │└ jle L289 ; │ @ transpose.jl:109 within `transpose_f!` vxorps %xmm0, %xmm0, %xmm0 vmovups %xmm0, 32(%rsp) movabsq $"transposeblock!", %rax callq *%rax jmp L711 ; │ @ transpose.jl within `transpose_f!` L289: movq %r12, -64(%rbp) ; │ @ transpose.jl:102 within `transpose_f!` ; │┌ @ range.jl:833 within `iterate` ; ││┌ @ range.jl:609 within `isempty` ; │││┌ @ operators.jl:378 within `>` ; ││││┌ @ int.jl:83 within `<` testq %r9, %r9 ; │└└└└ je L711 ; │ @ transpose.jl within `transpose_f!` testq %r8, %r8 ; │ @ transpose.jl:103 within `transpose_f!` je L711 ; │ @ transpose.jl within `transpose_f!` movq (%rdx), %rcx movq -72(%rbp), %rax movq (%rax), %r11 ; │ @ transpose.jl:104 within `transpose_f!` leaq 96(%r11), %rbx leaq 96(%rcx), %rsi leaq (,%r8,8), %rax movq %rax, -144(%rbp) movq %r8, %r10 andq $-16, %r10 movq -64(%rbp), %rax leaq (,%rax,8), %r13 movq %rcx, -88(%rbp) leaq -8(%rcx), %rax leaq 1(%r8), %rdi movl $1, %ecx xorl %r14d, %r14d movq %r11, -80(%rbp) movq %r9, -152(%rbp) movq -152(%rbp), %r9 jmp L466 nopw %cs:(%rax,%rax) ; │ @ transpose.jl within `transpose_f!` L416: movq -160(%rbp), %rcx ; │ @ transpose.jl:104 within `transpose_f!` ; │┌ @ range.jl:837 within `iterate` leaq 1(%rcx), %r8 ; │└ incq %r14 addq $8, %rbx movq -144(%rbp), %rdx addq %rdx, %rsi addq $8, %r11 addq %rdx, %rax ; │┌ @ range.jl:837 within `iterate` ; ││┌ @ promotion.jl:468 within `==` cmpq %r9, %rcx movq %r8, %rcx movq %r12, %r8 ; │└└ je L711 ; │ @ transpose.jl within `transpose_f!` L466: movq %rcx, -160(%rbp) movl $1, %r15d movq %r8, %r12 ; │ @ transpose.jl:104 within `transpose_f!` cmpq $16, %r8 jb L672 ; │ @ transpose.jl within `transpose_f!` movl $1, %r15d ; │ @ transpose.jl:104 within `transpose_f!` cmpq $1, -64(%rbp) jne L672 ; │ @ transpose.jl within `transpose_f!` movq %r12, %r15 imulq %r14, %r15 leaq (%r12,%r15), %rcx movq %r12, %r8 movq -88(%rbp), %rdx leaq (%rdx,%rcx,8), %r12 movq -80(%rbp), %rcx leaq (%rcx,%r14,8), %rcx ; │ @ transpose.jl:104 within `transpose_f!` cmpq %r12, %rcx jae L578 ; │ @ transpose.jl within `transpose_f!` movq -88(%rbp), %rcx leaq (%rcx,%r15,8), %r12 leaq (%r8,%r14), %rcx movq -80(%rbp), %rdx leaq (%rdx,%rcx,8), %rcx movl $1, %r15d ; │ @ transpose.jl:104 within `transpose_f!` cmpq %rcx, %r12 movq %r8, %r12 jb L672 L578: leaq 1(%r10), %r15 xorl %r12d, %r12d nopl (%rax) ; │┌ @ array.jl:862 within `getindex` L592: vmovups -96(%rsi,%r12,8), %ymm0 vmovups -64(%rsi,%r12,8), %ymm1 vmovups -32(%rsi,%r12,8), %ymm2 vmovups (%rsi,%r12,8), %ymm3 ; │└ ; │┌ @ array.jl:905 within `setindex!` vmovups %ymm0, -96(%rbx,%r12,8) vmovups %ymm1, -64(%rbx,%r12,8) vmovups %ymm2, -32(%rbx,%r12,8) vmovups %ymm3, (%rbx,%r12,8) addq $16, %r12 cmpq %r12, %r10 jne L592 ; │└ ; │ @ array.jl within `transpose_f!` movq %r8, %r12 ; │ @ transpose.jl:104 within `transpose_f!` cmpq %r10, %r8 je L416 nopl (%rax,%rax) L672: leaq -1(%r15), %rcx imulq -64(%rbp), %rcx leaq (%r11,%rcx,8), %rcx nopl (%rax) ; │┌ @ array.jl:862 within `getindex` L688: movq (%rax,%r15,8), %rdx ; │└ ; │┌ @ array.jl:905 within `setindex!` movq %rdx, (%rcx) ; │└ ; │┌ @ range.jl:837 within `iterate` incq %r15 ; ││┌ @ promotion.jl:468 within `==` addq %r13, %rcx cmpq %r15, %rdi ; │└└ jne L688 jmp L416 ; │ @ transpose.jl within `transpose_f!` L711: movq -120(%rbp), %rax movq -136(%rbp), %rcx movq %rax, (%rcx) movq -72(%rbp), %rax ; │ @ transpose.jl:111 within `transpose_f!` addq $168, %rsp popq %rbx popq %rdi popq %rsi popq %r12 popq %r13 popq %r14 popq %r15 popq %rbp vzeroupper retq ; │ @ transpose.jl:97 within `transpose_f!` ; │┌ @ range.jl:1045 within `==` @ promotion.jl:468 L752: cmpq %r8, %rbx je L236 jmp L153 nop ; └└ ```

You can compare your work to whatever you want, but you do need to compare your work to existing work and explain why that does not meet your needs. Perhaps the main thing to emphasize here is that these other numerical package are not written in or otherwise directly usable from C++. You can then explicitly contrast with Eigen in that while it is written in C++, it's API differs from MATLAB. Software does not exist in vacuum. Others have sought to solve the problem of wanting MATLAB semantics in an open source and faster executing environment. Please acknowledge prior efforts and differentiate your effort from those efforts using the points you have outlined. I can only review that section after it has been written.

rosoport commented 2 years ago

Hi madam/sir I really appreciate the invitation, but unfortunately I must decline it. Best regards Rodrigo Portugal

On Wed, Dec 8, 2021, 9:05 AM Mehmet Hakan Satman @.***> wrote:

@rosoport https://github.com/rosoport - This is the last call. I have not received a response from you either here or by e-mail. Please accept the invitation, if appropriate, by Saturday, December 11th. Otherwise I will have to find a new reviewer. For your information, best.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3965#issuecomment-988753697, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIGWVNTXGDEBTIPZSLZHHQDUP5CXPANCNFSM5JDT4MGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

jbytecode commented 2 years ago

@whedon remove @rosoport as reviewer

whedon commented 2 years ago

OK, @rosoport is no longer a reviewer

jbytecode commented 2 years ago

Dear all,

Because of our second reviewer has resigned, I am going to find a new reviewer. I am using email for invitations to keep here clean. I am first trying to invite the suggested reviewers by the author(s). Whenever I receive an acceptance, the new reviewer will be assigned.

While I am completing these stuff, the reviewing process can continue.

Statement of Need is the required section in JOSS submissions. State of the field can be a separate section or author(s) should describe how this software compares to other commonly-used packages in somewhere else (possibly in introduction) in the manuscript.

Thank you in advance.

jbytecode commented 2 years ago

@whedon add @pitsianis as reviewer

whedon commented 2 years ago

OK, @pitsianis is now a reviewer

jbytecode commented 2 years ago

@whedon generate pdf

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

mkitti commented 2 years ago

I'm working through the install on Windows and Ubuntu.

The order of the installation instructions is confusing since the instructions to install dependencies comes after the initial build instructions. Additionally, the instructions for macOS and Ubuntu are intertwined, while the Windows instructions are distinct.

Perhaps there should be separate macOS and Ubuntu sections so that someone can pick an operating system and then follow a linear, chronological set of instructions?

The macOS instructions seem quite home convenient as homebrew seems to do most of the work.

The Ubuntu version of the instruction involves building VTK from source. It is not clear form the instructions what versions have been tested. The authors only mention 9.x.x in the instructions for Linux. Confusingly, in the Windows instructions, apparently version 8.2 is fine.

VTK does take some time to build with the instructions given. Another thought would be to provide conda-forge based instructions since conda-forge does have version VTK 9.1 available across multiple platforms: https://anaconda.org/conda-forge/vtk

Perhaps contributing a conda-forge feedstock may provide a cross-platform way of distributing this software.

mkitti commented 2 years ago

On a Ubuntu 18.04 machine I get the following errors while trying to compile the demo:

$ cmake -DCMAKE_INSTALL_PREFIX=/home/mkitti/src/castor-install ..
-- Building castor v1.0.0
-- BLAS and LAPACK found
-- BLAS_LIBRARIES : /usr/lib/x86_64-linux-gnu/libopenblas.so
-- LAPACK_LIBRARIES : /usr/lib/x86_64-linux-gnu/libopenblas.so-lpthread-lm-ldl
-- Find VTK version : 9.1.0
-- VTK_LIBRARIES    : VTK::ChartsCore;VTK::CommonColor;VTK::CommonCore;VTK::CommonDataModel;VTK::InteractionStyle;VTK::RenderingContext2D;VTK::RenderingContextOpenGL2;VTK::RenderingCore;VTK::RenderingFreeType;VTK::RenderingGL2PSOpenGL2;VTK::RenderingOpenGL2;VTK::ViewsContext2D;VTK::RenderingAnnotation;VTK::IOImage;VTK::IOPLY;VTK::IOLegacy;VTK::IOOggTheora;VTK::FiltersGeometry
-- Configuring done
-- Generating done
-- Build files have been written to: /home/mkitti/src/castor/build
(base) mkitti@woodward:~/src/castor/build$ make install
Consolidate compiler generated dependencies of target test
[ 18%] Built target test
Consolidate compiler generated dependencies of target demo_matrix
[ 27%] Built target demo_matrix
Consolidate compiler generated dependencies of target demo_smatrix
[ 36%] Built target demo_smatrix
[ 40%] Building CXX object demo/CMakeFiles/demo_hmatrix.dir/demo_hmatrix.cpp.o
In file included from /home/mkitti/src/castor/demo/demo_hmatrix.cpp:17:0:
/home/mkitti/src/castor/include/castor/linalg.hpp: In function ‘void castor::tgemm(R, const castor::matrix<std::complex<float> >&, const castor::matrix<std::complex<float> >&, S, castor::matrix<std::complex<float> >&)’:
/home/mkitti/src/castor/include/castor/linalg.hpp:58:104: error: cannot convert ‘std::complex<float>*’ to ‘const float*’ for argument ‘7’ to ‘void cblas_cgemm(CBLAS_ORDER, CBLAS_TRANSPOSE, CBLAS_TRANSPOSE, blasint, blasint, blasint, const float*, const float*, blasint, const float*, blasint, const float*, float*, blasint)’
                 &alpha_c, &A(0), (int)size(A,2), &B(0), (int)size(B,2), &beta_c, &C(0), (int) size(C,2));
                                                                                                        ^
/home/mkitti/src/castor/include/castor/linalg.hpp: In function ‘void castor::tgemm(R, const castor::matrix<std::complex<double> >&, const castor::matrix<std::complex<double> >&, S, castor::matrix<std::complex<double> >&)’:
/home/mkitti/src/castor/include/castor/linalg.hpp:66:104: error: cannot convert ‘std::complex<double>*’ to ‘const double*’ for argument ‘7’ to ‘void cblas_zgemm(CBLAS_ORDER, CBLAS_TRANSPOSE, CBLAS_TRANSPOSE, blasint, blasint, blasint, const double*, const double*, blasint, const double*, blasint, const double*, double*, blasint)’
                 &alpha_z, &A(0), (int)size(A,2), &B(0), (int)size(B,2), &beta_z, &C(0), (int) size(C,2));
                                                                                                        ^
In file included from /home/mkitti/src/castor/demo/demo_hmatrix.cpp:17:0:
/home/mkitti/src/castor/include/castor/linalg.hpp: In function ‘auto castor::aca(castor::matrix<long unsigned int>, castor::matrix<long unsigned int>, const std::function<castor::matrix<std::complex<double> >(castor::matrix<long unsigned int>, castor::matrix<long unsigned int>)>&, double, std::size_t, bool)’:
/home/mkitti/src/castor/include/castor/linalg.hpp:1573:76: error: cannot convert ‘std::complex<double>*’ to ‘const double*’ for argument ‘5’ to ‘void cblas_zgemv(CBLAS_ORDER, CBLAS_TRANSPOSE, blasint, blasint, const double*, const double*, blasint, const double*, blasint, const double*, double*, blasint)’
                     &cb_mun, &b[0], (int) Nc, &u[0], 1, &cb_un, &bnp1[0], 1);
                                                                            ^
/home/mkitti/src/castor/include/castor/linalg.hpp:1591:76: error: cannot convert ‘std::complex<double>*’ to ‘const double*’ for argument ‘5’ to ‘void cblas_zgemv(CBLAS_ORDER, CBLAS_TRANSPOSE, blasint, blasint, const double*, const double*, blasint, const double*, blasint, const double*, double*, blasint)’
                     &cb_mun, &a[0], (int) Nr, &v[0], 1, &cb_un, &anp1[0], 1);
                                                                            ^
/home/mkitti/src/castor/include/castor/linalg.hpp:1602:75: error: cannot convert ‘std::complex<double>*’ to ‘const double*’ for argument ‘5’ to ‘void cblas_zgemv(CBLAS_ORDER, CBLAS_TRANSPOSE, blasint, blasint, const double*, const double*, blasint, const double*, blasint, const double*, double*, blasint)’
                     &cb_mun, &a[0], (int) Nr, &anp1[0], 1, &cb_z, &u[0], 1);
                                                                           ^
/home/mkitti/src/castor/include/castor/linalg.hpp:1604:75: error: cannot convert ‘std::complex<double>*’ to ‘const double*’ for argument ‘5’ to ‘void cblas_zgemv(CBLAS_ORDER, CBLAS_TRANSPOSE, blasint, blasint, const double*, const double*, blasint, const double*, blasint, const double*, double*, blasint)’
                     &cb_mun, &b[0], (int) Nc, &bnp1[0], 1, &cb_z, &v[0], 1);
                                                                           ^
demo/CMakeFiles/demo_hmatrix.dir/build.make:75: recipe for target 'demo/CMakeFiles/demo_hmatrix.dir/demo_hmatrix.cpp.o' failed
make[2]: *** [demo/CMakeFiles/demo_hmatrix.dir/demo_hmatrix.cpp.o] Error 1
CMakeFiles/Makefile2:303: recipe for target 'demo/CMakeFiles/demo_hmatrix.dir/all' failed
make[1]: *** [demo/CMakeFiles/demo_hmatrix.dir/all] Error 2
Makefile:135: recipe for target 'all' failed
make: *** [all] Error 2
whedon commented 2 years ago

:wave: @mkitti, please update us on how your review is going (this is an automated reminder).

mkitti commented 2 years ago

I just got my MSYS2 on Windows into decent shape, so I will try that again shortly.

We probably need more specific information regarding what compilers and what versions have been tested. It may also be necessary to specify to cmake the required the C++ standard to use.

jbytecode commented 2 years ago

@matthieuaussal - It seems we have some reviewer issues, could you please update your status?

mkitti commented 2 years ago

See https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html

mkitti commented 2 years ago

@jbytecode Could you outline the "State of the Field" requirements here? The only comparison is to the MATLAB, and I think there are other open source alternatives that try to address the stated problem, albeit in distinct ways. In reviewing the requirements with you, we determined that this perhaps does not need to be a distinct section, but the content does need to be clearly somewhere in the manuscript - likely part of "Statement of Need".

jbytecode commented 2 years ago

State of the field can be a distinct section or the authors should describe how this software compares to other commonly-used packages in somewhere else in the paper, but yes, this comparison is somehow required.

matthieuaussal commented 2 years ago

We just made this comparison between Matlab, Julia, Eigen and Castor and we planned to add this bench in the paper next tuesday, in a new "state of the field" part.

But you can already have a look at these attached codes.

Have a nice we

Le 17 déc. 2021 à 16:46, Mehmet Hakan Satman @.***> a écrit :

State of the field can be a distinct section or the authors should describe how this software compares to other commonly-used packages in somewhere else in the paper, but yes, this comparison is somehow required.

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3965#issuecomment-996824662, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHFHRGBENQYJNOHIQXYXVNTURNLOLANCNFSM5JDT4MGQ. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were mentioned.