search

openjournals / joss-reviews

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

[REVIEW]: GALAHAD 4.0: an open source library of Fortran packages with C and Matlab interfaces for continuous optimization #4882

Closed editorialbot closed 1 year ago

editorialbot commented 2 years ago

Submitting author: !--author-handle-->@nimgould<!--end-author-handle-- (Nicholas Gould) Repository: https://github.com/ralna/GALAHAD Branch with paper.md (empty if default branch): Version: v4.0.0 Editor: !--editor-->@melissawm<!--end-editor-- Reviewers: @frankecurtis, @jajhall Archive: 10.5281/zenodo.8075232

Status

status

Status badge code:

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

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

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

@editorialbot generate my checklist

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

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

Checklists

📝 Checklist for @frankecurtis

📝 Checklist for @jajhall

editorialbot commented 2 years ago

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

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

@editorialbot commands

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

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

OK DOIs

- 10.1007/0-387-30065-1_4 is OK
- 10.1007/978-3-662-12211-2 is OK
- 10.1145/962437.962438 is OK
- 10.1007/s10589-014-9687-3 is OK
- 10.1007/s10107-004-0559-y is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 2 years ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=3.33 s (428.2 files/s, 322250.1 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Fortran 90                      849         136012         153489         481626
TeX                             102          14676           3869          77591
Fortran 77                       72            414          53521          41858
C                                99           3160           2793          18021
C/C++ Header                    102           8649          30856          10984
Ruby                              2              0              0           6862
Python                           10           1215            635           5443
Bourne Again Shell               24           1105           1220           3800
CUDA                              6            503            454           3476
C++                              13            287            918           2779
MATLAB                           94            345           2661           1776
reStructuredText                  7            349             35           1349
HTML                              2              4              0           1056
sed                              33              2              0            174
Markdown                          2             16              0            136
C Shell                           4             46            122             70
Bourne Shell                      2             43             51             65
make                              2             13             21             26
YAML                              1              1              4             18
CSS                               1              0              1             10
D                                 1              0              0              1
--------------------------------------------------------------------------------
SUM:                           1428         166840         250650         657121
--------------------------------------------------------------------------------

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

Wordcount for paper.md is 1143

editorialbot commented 2 years ago

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

melissawm commented 2 years ago

:wave: @nimgould @frankecurtis @jajhall this is the issue where the review will take place. If you have questions, please let me know. Cheers!

danielskatz commented 1 year ago

@melissawm - after about 4 weeks, can you check on progress here?

melissawm commented 1 year ago

Thanks for the reminder @danielskatz - @nimgould @frankecurtis @jajhall any updates?

frankecurtis commented 1 year ago

@melissawm sorry for the delay. It had reached the top of my stack, but then fell off when the review wasn't open. But I'm ready to go and will complete it tomorrow (November 23). A nice U.S. Thanksgiving present to @nimgould even if he doesn't celebrate it all the way over in the UK ;-)

frankecurtis commented 1 year ago

Review checklist for @frankecurtis

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

frankecurtis commented 1 year ago

@melissawm @nimgould I am done with my review.

I previously communicated to @nimgould about an issue that I had during the installation process (since I was following the Homebrew instructions, which are apparently out of date). I did not submit an issue about this to the GALAHAD repository, but @nimgould is aware of it nonetheless.

In terms of the review section "Documentation > A statement of need", I suppose I was expecting to see the statement of need written more upfront in the documentation; instead, I only found it here: https://github.com/ralna/GALAHAD/blob/master/doc/paper_v4/paper.md Still, it was present, so I checked the box.

Finally, I did encounter one issue when running the general tests for my installation. I submitted an issue on the GALAHAD repository. But it doesn't seem necessary to block the review for this. Overall, my review is complete. Thanks.

melissawm commented 1 year ago

Thank you @frankecurtis !

@jajhall any updates? Let me know if you need help setting up your checklist or if you need more information on the review. Thanks!

jajhall commented 1 year ago

Sorry I've been travelling a lot recently, but will be home for the next fortnight and will see that I have done the review by 16 December

danielskatz commented 1 year ago

👋 @jajhall - just a ping on this. Please remember you need to create a review checklist by runing this command in a separate comment: @editorialbot generate my checklist

jajhall commented 1 year ago

Review checklist for @jajhall

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

jajhall commented 1 year ago

👋 @jajhall - just a ping on this. Please remember you need to create a review checklist by runing this command in a separate comment: @editorialbot generate my checklist

Sorry I've not fulfilled my promise to get this done by 16 December. Once I stopped being busy I just had to rest

nimgould commented 1 year ago

Thanks, Julian. As I and much of the rest of the world are in hibernation until early January, don't ruin your downtime with such trifles before then.

Happy new year to all

Nick -- Nicholas I. M. Gould Scientific Computing Department email: @.*** G39, R71 Rutherford Appleton Laboratory phone: 0(44)1235 445801 Chilton, Oxon OX11 0QX England EU www: http://www.numerical.rl.ac.uk/people/nimg

This email and any attachments are intended solely for the use of the named recipients. If you are not the intended recipient you must not use, disclose, copy or distribute this email or any of its attachments and should notify the sender immediately and delete this email from your system. UK Research and Innovation (UKRI) has taken every reasonable precaution to minimise risk of this email or any attachments containing viruses or malware but the recipient should carry out its own virus and malware checks before opening the attachments. UKRI does not accept any liability for any losses or damages which the recipient may sustain due to presence of any viruses.

danielskatz commented 1 year ago

👋 @melissawm - It seems like something is stalled here - can you let me know what it is, and how we restart it?

melissawm commented 1 year ago

I believe we are pending on @jajhall 's review - are there any updates @nimgould ?

nimgould commented 1 year ago

No, I haven't heard a thing. Nick

On 3/7/23 15:42, Melissa Weber Mendonça wrote:

I believe we are pending on @jajhall https://github.com/jajhall 's review - are there any updates @nimgould https://github.com/nimgould ?

— Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/4882#issuecomment-1458389790, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACW4A6WRVN3AOLXSXKA32O3W25JNZANCNFSM6AAAAAARO5WT2Q. You are receiving this because you were mentioned.Message ID: @.***>

-- Nicholas I. M. Gould Scientific Computing Department email: @.*** G39, R71 Rutherford Appleton Laboratory phone: 0(44)1235 445801 Chilton, Oxon OX11 0QX England EU www: http://www.numerical.rl.ac.uk/people/nimg

nimgould commented 1 year ago

I had expected something in January, I must admit

danielskatz commented 1 year ago

👋 @jajhall - can you let us know when to expect something from you?

danielskatz commented 1 year ago

@melissawm - we might need to find an additional reviewer if we don't hear back from @jajhall, though you might want to ping him by email first to make sure he's getting github notifications.

jajhall commented 1 year ago

I'm on it now. Work and personal life have been rather hectic in the past few months. [Mother died towards the end of January. No big deal in itself, but created a lot to do on top of work as I'm an only child.]

danielskatz commented 1 year ago

@jajhall - I apologize for bugging you at this time and offer my sympathies on your loss

nimgould commented 1 year ago

@jajhall Very sorry to hear that, Julian. My mother passed away 18 months away, and I can imagine what you are/have been going through (funeral and probate to start with, not to mention loss)

melissawm commented 1 year ago

@jajhall I'm so very sorry for your loss. Please let us know if there's anything we can do to make it easier.

jajhall commented 1 year ago

Thanks for your concerns, but don't worry everyone. Mum was very old and ready to go. It's just made me busier than usual. 🙏

jajhall commented 1 year ago

I'm working through the checklist, and have now installed Galahad. Am I right in thinking that I should produce a report expanding on my views/experience of the checklist items?

danielskatz commented 1 year ago

Hi @jajhall - just to step in quickly (and @melissawm should also comment if she wants), if something on the checklist is satisfied, you don't really need to say anything about it, though if you want to provide non-binding suggestions for improvements, that's fine. If something is not satisfied, however, then yes, you do need to explain what you see the problem as, and perhaps how it might be solved so that you can check the item. Best practice is typically to open an issue in the repo of the code being reviewed, and to mention https://github.com/openjournals/joss-reviews/issues/4882 in it, so that a link appears here that shows the status of the issue. In some cases, you could also create a PR in the repo (and again mention this review issue) if it's easier to show the potential solution/fix that way.

danielskatz commented 1 year ago

👋 @jajhall - Sorry, I don't want to rush you, but I would like to see what progress has been made in the month since the last comments above, and when you think you might be done with your review. JOSS normally tries to get reviews done in 2-6 weeks, and this one is taking quite a bit longer, though some of the reason is quite understandable.

jajhall commented 1 year ago

Hi @danielskatz . My teaching for the year finished yesterday, so I'm now free to do things like this. I won't delay unduly

jajhall commented 1 year ago

Installation is OK, but complex and opaque, even on a standard Linux machine. Thus I dispute that "GALAHAD is easy to install using its own make-based system". I had to abandon my first attempt (I think due to an error in an environment variable) and reinstall from scratch.

I did get the software to solve some problems, although there were some failures that yielded status codes that were opaque until I was referred to the relevant pdf file in https://github.com/ralna/GALAHAD/tree/v4.0.0/doc

Documentation

In the Documentation there is no direct "statement of need". This only appears in the paper, which is available from within the documentation as https://github.com/ralna/GALAHAD/blob/v4.0.0/doc/paper_v4/paper.md.

I see no Community guidelines:. Even if there is no expectation of community contribution, this should be stated - with explanation. Given the use of GitHub, there is no need to document how to Report issues or problems with the software or Seek support

Software paper

In the Summary, the "description of the high-level functionality and purpose of the software for a diverse, non-specialist audience" is very terse indeed, but understandable given the scope of the software.

In its Statement of need, the paper states that "we have now provided interfaces from Fortran to C for a significant subset of the Fortran packages." Surely the interfaces are from C to Fortran.

In the context of State of the field, another "Rival open-source solver" is mentioned, and one commercial solver, but there is no "comparison"

I'm surprised to see no (link) to performance comparisons such as http://plato.asu.edu/ftp/qpbench.html

jajhall commented 1 year ago

Sorry to have taken so long to review this article.

nimgould commented 1 year ago

Thank you. In response

  1. I had intended that the README provides many of the details of what is required from the user to install things successfully, and to use it afterwards. Once the pieces (optional software, etc) are in place, the install script aims to guide the user to multi-compiler (and possibly multi-platform) installs from a single point. Complications are generally in choosing which subsets of the library to install, with which operating system(s), which compilers and with which other language interfaces.

  2. Of course, we have no expectation that the software is bug free, or even that it will succeed in all cases (that is the nature of nonlinear problems). We do try to respond to any issues, and there is an active issues page on github page. There is ongoing community collaboration, particularly on moving beyond C to other supported languages such as Python and Julia that is as a result of the C interfaces.

  3. I had assumed that the "need" was obvious, and justified ever since the original GALAHAD paper in TOMS. I had intended that the v4 paper referred to would be the modern equivalent. But perhaps I am wrong. Certainly, the suggestion from JOSS was that this should not be as detailed as might be expected in a full journal paper.

  4. The "interface to" seems to be used in both directions on the web. I will change this to "between" to keep both sides happy.

  5. You are right, there is no global comparison with other software. Individual packages have been compared (at the time) in the articles that gave rise to them, but no attempt has been made to keep this up to date, the team is simply too small.

  6. Specifically on http://plato.asu.edu/ftp/qpbench.html, there used to be (a decade ago) some comparisons, but the site maintainer stopped supporting fortran. However, I just looked again., and comparison with GALAHAD's CQP has reappeared; the code seems to be generally reliable, and reasonably efficient in comparison with the competition. Those examples for which CQP fails are well understood, and related to the default sparse linear-equation solver used. Should we mention this, it is just one of roughly 100 GALAHAD packages?

Thank you very much for the review and comments, they have been very useful, and some changes have been made to the planned version 5 as a consequence.

melissawm commented 1 year ago

Hi all - since I see all items have been ticked on the checklist I am assuming the reviewers are happy with the current state of the package, and that we can move forward with acceptance. Is that so? @jajhall @frankecurtis

Can I ask you to take one last look at the paper @nimgould and see if there are any pending fixes you'd like to make?

Thanks!

frankecurtis commented 1 year ago

@melissawm yes

jajhall commented 1 year ago

Yes

nimgould commented 1 year ago

Mellissa, I'm out of town (on holiday) at the moment, I do have a couple of small changes to make. I'll get back to you early next week. Frank + Julian ... thank you!

nimgould commented 1 year ago

I have now made one small change to the paper (replace the phrase "from Fortran to C" to "between Fortran and C" as suggested by Julian Hall. That is all from me. Many thanks to both reviewers and Mellissa.

nimgould commented 1 year ago

This has all gone quiet, I hope that things are happening behind the scenes ...

melissawm commented 1 year ago

@editorialbot generate pdf

editorialbot commented 1 year ago

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

melissawm commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1007/978-3-662-12211-2 is OK
- 10.1145/962437.962438 is OK
- 10.1007/s10589-014-9687-3 is OK
- 10.1007/s10107-004-0559-y is OK

MISSING DOIs

- None

INVALID DOIs

- 10.1007/0-387-30065-1\_4 URL is INVALID
melissawm commented 1 year ago

@nimgould could you fix the invalid DOI pointed above (for KNITRO)? A small note (not blocking, but might be worth checking) is that the first column on the table on page 2 could have a lower width, making the text fit better on the page. But it is legible as it is 🙂

Once you fix that and you are happy with the result, can you make a tagged release for GALAHAD and archive it on zenodo, reporting the version number and archive DOI as a comment here? Thanks!

nimgould commented 1 year ago

@melissawm I have uploaded the most recent version (4.1 that includes bug fixes and enhancements) of the software to zenodo, and it appears that it has a DOI 10.5281/zenodo.8075232

I couldn't get the table columns to shrink, this seems to be a defect for md files and much commented on in google. Sorry!

danielskatz commented 1 year ago

@melissawm - It looks like this is ready to move forward, I think

melissawm commented 1 year ago

@editorialbot check references

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

OK DOIs

- 10.1007/0-387-30065-1_4 is OK
- 10.1007/978-3-662-12211-2 is OK
- 10.1145/962437.962438 is OK
- 10.1007/s10589-014-9687-3 is OK
- 10.1007/s10107-004-0559-y is OK

MISSING DOIs

- None

INVALID DOIs

- None