search

openjournals / joss-reviews

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

[REVIEW]: qgs: A flexible Python framework of reduced-order multiscale climate models #2597

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @jodemaey (Jonathan Demaeyer) Repository: https://github.com/Climdyn/qgs Version: v0.2.0 Editor: @harpolea Reviewer: @eviatarbach, @sadielbartholomew Archive: 10.5281/zenodo.4368844

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

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

@eviatarbach & @sadielbartholomew, 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 @harpolea 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 @eviatarbach

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @sadielbartholomew

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @eviatarbach, @sadielbartholomew 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 4 years ago

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

Can't find any papers to compile :-(

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

eviatarbach commented 4 years ago

qgs provides a modular implementation of a quasi-geostrophic atmospheric model. Having previously used the MAOOAM model in research, I was impressed with its ease-of-use, highly readable code, the elegance of the tensor-based implementation, and built-in tangent linear and adjoint models. qgs builds on this by adding two different land components, and making it easy to switch between ocean and land coupling. qgs is also written entirely in Python, as opposed to MAOOAM that had Fortran and Lua implementations also. I commend the authors for their excellent contribution and hope that qgs will become a widely used tool in the modelling hierarchy.

Suggested changes:

  1. There are a few missing parts of the documentation, as per the review checklist. The statement of need is provided in the software paper, but it has to be in the documentation as well.
    • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
    • 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
  2. I would like to see the software paper mention the availability of the tangent linear and adjoint models, as this is a major advantage for data assimilation research, as well as for, e.g., computing the Lyapunov spectrum. Adding an example of the usage of the tangent linear or adjoint models to the documentation would be helpful.
  3. This implementation is written in Python instead of Fortran, but uses Numba and sparse matrices to accelerate the computations. I would thus like to see a performance comparison to the Fortran version of MAOOAM, either in the documentation or the software paper (see the performance item on the checklist).

Minor issues:

kthyng commented 3 years ago

Hi @jodemaey! I am the rotating associate editor in chief this week and am going through stale submissions. I see that one of your reviewers put forth some comments above but you haven't responded yet. Will you be able to get to this soon? If not, that is ok, but we should in that case pause this submission. I recommend that we pause this if we haven't heard back from the author within a week or so.

sadielbartholomew commented 3 years ago

Regarding your comment @kthyng, just to clarify that I was waiting on the submitting author's response to the first reviewer's comments, as in https://github.com/openjournals/joss-reviews/issues/2597#issuecomment-683972381, before starting my review, in case there were any significant changes made to the codebase or paper based on that feedback.

However, as you state it has been a while now, a good month, since that feedback without response. Would you recommend I start my review regardless, now? Or wait considering this could be considered 'stale'? I am just wary we are approaching six weeks since the review was opened and it is recommended I complete my review within that timeframe.

jodemaey commented 3 years ago

Hi @kthyng and @sadielbartholomew,

There was a misunderstanding here. I was waiting for the second review to address all the comments at once.

In any case I'm not able to work on this until the end of October. At that time I will work again fully on qgs and will address the comments and suggestions of @eviatarbach .

sadielbartholomew commented 3 years ago

Sorry that there was this misunderstanding, @jodemaey.

If you are unable to make changes until November it sounds like it would be best for me to complete my review by then so that you can know what else, if anything, might need to be addressed. I will ensure I have my review completed by then, and try to do it much sooner anyhow, now that I know about the situation. Thanks.

kthyng commented 3 years ago

@jodemaey, @sadielbartholomew I'm glad I commented then to clear this up!

@sadielbartholomew sounds good to me. I will not pause this submission then since we'll expect to hear from you in the next month with your review, and then subsequently from @jodemaey with responses.

Thanks all!

jodemaey commented 3 years ago

@sadielbartholomew , I am also sorry about this, I should have replied to Eviatar's review to clarify this. Thank you in advance for your review.

@kthyng Thank you.

arfon commented 3 years ago

:wave: @sadielbartholomew - just checking you're planning on completing your review in the next week or so?

sadielbartholomew commented 3 years ago

Hi @arfon, yes, I will complete a first review over the weekend. Thanks for your patience.

sadielbartholomew commented 3 years ago

Really sorry for the further delay, I am having to co-manage an urgent operational infrastructure situation. I should have some time to review this later tonight and tomorrow so can complete my review by Saturday morning (UK time).

sadielbartholomew commented 3 years ago

@whedon commands

whedon commented 3 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository
sadielbartholomew commented 3 years ago

@whedon check repository

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.84  T=0.36 s (159.6 files/s, 37541.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
SVG                              4              4              4           2937
Python                          17           1058           1792           2480
reStructuredText                20            373            387            454
Jupyter Notebook                 8              0           3037            439
TeX                              1             34              0            385
Markdown                         1             46              0             97
DOS Batch                        1              8              1             26
YAML                             2              5              4             23
Bourne Shell                     3              8              0             15
make                             1              4              9              9
-------------------------------------------------------------------------------
SUM:                            58           1540           5234           6865
-------------------------------------------------------------------------------

Statistical information for the repository 'ce242648c96bcd1558042443' was
gathered on 2020/11/06.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Jonathan Demaeyer               18          6983           1653          100.00

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
Jonathan Demaeyer          5330           76.3          4.0                6.45
jodemaey commented 3 years ago

Hi @sadielbartholomew ,

I have seen that you raised two issues in the qgs repo. Does that mean that your review is complete?

Thank you in advance,

Jonathan

sadielbartholomew commented 3 years ago

Hi @jodemaey, no I only got part way through my review in the time I could devote last week. I will try to finish the review tonight and tomorrow. Sorry for the delay and thanks for your patience.

arfon commented 3 years ago

:wave: @sadielbartholomew - just checking in here to see how you're getting on?

sadielbartholomew commented 3 years ago

Sorry @arfon I still need a bit more time here. Let me promise that by the end of this week (Sunday 29th, UK time) I will finish the review.

arfon commented 3 years ago

Sorry @arfon I still need a bit more time here. Let me promise that by the end of this week (Sunday 29th, UK time) I will finish the review.

:+1: no problem. Thanks for the update!

jodemaey commented 3 years ago

Sorry @arfon I still need a bit more time here. Let me promise that by the end of this week (Sunday 29th, UK time) I will finish the review.

Hi @sadielbartholomew ,

Thank you. Meanwhile, I've addressed your two issues on the qgs repo.

sadielbartholomew commented 3 years ago

@whedon generate pdf from branch joss

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss. Reticulating splines etc...
whedon commented 3 years ago

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

sadielbartholomew commented 3 years ago

Hi @jodemaey, I assume that the current state of the proposed JOSS paper content is contained in the joss branch rather than master? I've just re-generated it via the whedon tool, as per the above comments.

Please in future let us know when you update the paper, since I just spent a significant amount of time reviewing the old version only to see that it has changed quite a bit now, with a new section (that's okay, but going forward it helps for us to know of any state changes).

It's good in the respect that you have, with the updates since the last version generated and linked in this thread (https://github.com/openjournals/joss-reviews/issues/2597#issuecomment-679154289), addressed some aspects that I was going to raise, e.g. interlinking between the references in the text and their corresponding entries in the 'References' section.

However there are some minor aspects of the updated paper itself that I have feedback on. Since these are spread across the paper.md , it is clearest if I can refer to specific lines there, so I will use a Pull Request (which I'll open in a moment and link to this Issue) to provide this feedback. Feel free to close it or merge it, as you wish, but consider the contained changeset and comments upon it formal feedback on the paper content (i.e. towards the items listed uner 'Software paper' in the review checklist).

Thanks.

jodemaey commented 3 years ago

Hi @sadielbartholomew ,

Thanks for your suggestions. I will merge them to the joss branch when your review is finished.

jodemaey commented 3 years ago

Hi @sadielbartholomew ,

I had a look at your commit. Can I merge it as it is? What about the other items on the checklist about functionalities and documentation: have you considered them already?

sadielbartholomew commented 3 years ago

I had a look at your commit. Can I merge it as it is?

Hi @jodemaey, sorry I fell asleep before I put it in as a PR. Please don't merge it yet. Let me open it now since many of my comments are not contained in the commit but I intended to put as comments on said PR. One moment.

What about the other items on the checklist about functionalities and documentation: have you considered them already?

I am nearly ready to tick these but not quite since I want to be thorough. I'll let you know by the end of the day. Thanks.

sadielbartholomew commented 3 years ago

Just to note here that I am ticking off items from the review checklist based on the joss branch, rather than the master branch, as that has been updated in response to feedback and will be the branch used for the state of the codebase for the archive and for the paper content.

Notably, for reviewing the items under 'Documentation' I have built the documentation locally from the joss branch (via the documented procedure); I am not referring to the documentation currently hosted online.

sadielbartholomew commented 3 years ago

@jodemaey: regarding the installation, the documented approach to installation does work in that it allows the scripts to run successfully, so that is sufficient for the review checklist item. Saying that, I've raised an issue because going forward ideally the package could be made available to install from PyPI or some conda channel or similar.

I do however have to query a claim in the documentation on that note, namely as stated under the general_information.html#installation endpoint of the documentation (from the joss branch so built locally):

The easiest way to install is through Anaconda.

The installation documented there isn't by Anaconda, as conda is only being used to create and activate an environment with the required dependencies. Strictly nothing is being 'installed' at all as no package has been set up, rather we can run qgs as documented because the python interpreter is being run directly on the set of top-level scripts.

I think the wording of the documentation in this respect needs to be amended to reflect the fact that there is no 'installation' going on per se, else parts such as that sentence are confusing and untrue. An environment is set up with the dependencies, then the scripts, cloned locally, can be run. So for example, the quoted sentence above would in accuracy be something like "The easiest way to run qgs is to use an appropriate environment created through Anaconda" or similar.

jodemaey commented 3 years ago

@sadielbartholomew : I have responded to your remark about packaging the code on the qgs repo issues page: https://github.com/Climdyn/qgs/issues/4

jodemaey commented 3 years ago

Hi @sadielbartholomew ,

Please note that new modifications of the paper have been pushed to the joss branch in commit: https://github.com/Climdyn/qgs/commit/b19cbadc8020f8f77f7e451f35d922ff6d1a935c

sadielbartholomew commented 3 years ago

Thanks @jodemaey, that's all good. I'll continue with the review tomorrow or the day after, aiming to complete it.

jodemaey commented 3 years ago

Hi @sadielbartholomew ,

When can I reasonably expect your review to be final? I need that information to plan my week of work.

Thank you in advance.

sadielbartholomew commented 3 years ago

Hi @jodemaey, I'm not far off with finalising the review so will try tonight (early hours of Tuesday UTC) to report on anything I consider outstanding towards acceptance. If not, I'll definitely be done by Wednesday night. Thanks again for your patience.

sadielbartholomew commented 3 years ago

Thanks for opening up an issue indicating you are going to address my previous comment regarding the installation instructions, @jodemaey. I was going to re-raise that in case you missed it. Otherwise all of my feedback raised so far has been addressed well, bar one small formatting change to the text, described against point (3) below.

I have now completed the remainder of my review and I've outlined the results below (in short I'm happy with everything except for a small number of items I feel still need to be addressed). Thanks for your patience waiting for the rest of the review.

Final review comments

If the following three points are addressed satisfactorily then I am more than happy to tick the remaining items in the review checklist and moreover to recommend acceptance of the qgs paper to JOSS:

  1. Regarding the state of the field, the review criterion is to 'describe how this software compares to other commonly-used packages' and whilst several similar packages are currently referenced and briefly described under a section of that name, I can't find any comparison between qgs and any of those packages in the text. There is some indication of the relation to MAOOAM, but since that is your own package and is included in qgs, I wouldn't count it as a separate package.

    So, what I would like to see towards fulfilling that criterion is some brief comparison between qgs and the listed alternative "easy-to-use idealized atmospheric model[s]" as you introduce them (i.e. q-gcm, pyqg, Isca). A sentence or two on what qgs offers that these libraries do not, or vice versa, would suffice. It may tie in with ground you have already covered in the first two paragraphs of the 'Statement of need' section, so you could perhaps reference them there with some further detail to address this point.

  2. The functionality of the software is documented really well, which is great. I did feel that the API reference documentation is quite hard to find, though. For example, to see the model configuration classes I would have to go through the menu like so: 'Code Description -> Modules -> Configuration of the model' and then there's a bit of introductory text before the class signature and parameters are listed, so if a reader was viewing on a laptop they might not see it and not realise the classes were covered below.

    In my experience of using and creating software documentation, it is most common to have the API reference as a top-level menu item, under a name such as 'API' or 'reference', where it is evident and easy to access. Therefore I advise you:

    • move the 'Modules' section out to a higher-level in the documentation tree; and/or (but ideally 'and')
    • rename it to one of the names above, as 'Modules' could imply an overview or commentary on the modules rather than the API reference for them.

  3. For the paper text formatting, please convert "python" to "Python" in the Performance section of the paper text (as raised in Climdyn/qgs#3 but not yet addressed).

Finally, I have some further comments that are not compulsory for my review acceptance, but that I think would improve the software or the paper content:

  1. I notice the labels on both the bar charts, (a) and (b), in Figure 1 are identical, so to make the visual easier to comprehend I suggest removing the labels on the top chart and moving it so the bottom is joined to the top of the lower chart, or to convert it into a single bar chart with two bars per item on the x-axis.

  2. I've noticed that the repository is structured in a way that makes it difficult to search through, namely with the qgs functional modules all in separate directories at the root of the repo, instead of being contained within a directory underneath the root which is much more common. This means one can't use a simple git grep to search for keywords in the modules to aid with development, as it will pick up words from the Notebook .ipynb files (which are big JSON structures with lots of binary code to save the images etc. so clog up the terminal on a search) or the documentation source and build pages, etc., too.

    For that reason, so any contributors can easily work on the core code, personally I'd move all of the module directories under another dedicated directory called qgs or lib or similar.

jodemaey commented 3 years ago

Dear all,

Please find below our response to the reviews of the referees (see the posts https://github.com/openjournals/joss-reviews/issues/2597#issuecomment-683972381 and https://github.com/openjournals/joss-reviews/issues/2597#issuecomment-741610669 ).

Response to referee 1

We thank @eviatarbach for his helpful review. We respond below to the suggested changes:

qgs provides a modular implementation of a quasi-geostrophic atmospheric model. Having previously used the MAOOAM model in research, I was impressed with its ease-of-use, highly readable code, the elegance of the tensor-based implementation, and built-in tangent linear and adjoint models. qgs builds on this by adding two different land components, and making it easy to switch between ocean and land coupling. qgs is also written entirely in Python, as opposed to MAOOAM that had Fortran and Lua implementations also. I commend the authors for their excellent contribution and hope that qgs will become a widely used tool in the modelling hierarchy.

Suggested changes:

  1. There are a few missing parts of the documentation, as per the review checklist. The statement of need is provided in the software paper, but it has to be in the documentation as well.
    • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
    • 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

We have added a statement of need section in the documentation. We have also added community guidelines on how to contribute to the development. These additions are contained in the commit f87022c6 in the joss branch.

  1. I would like to see the software paper mention the availability of the tangent linear and adjoint models, as this is a major advantage for data assimilation research, as well as for, e.g., computing the Lyapunov spectrum. Adding an example of the usage of the tangent linear or adjoint models to the documentation would be helpful.

We now mention the tangent linear explicitly in the paper. We have also added a usage example in the documentation. Thank you for the suggestion. The commits of these additions are cee79b7b and 9fd689d8 .

  1. This implementation is written in Python instead of Fortran, but uses Numba and sparse matrices to accelerate the computations. I would thus like to see a performance comparison to the Fortran version of MAOOAM, either in the documentation or the software paper (see the performance item on the checklist).

We have now made a benchmark of qgs versus MAOOAM lua/fortran code. The results are detailed in a new section of the paper. Related commit: dfaeed97 .

Minor issues:

  • "Simpy" should be Sympy in the forthcoming developments section.
  • The link to the notebooks folder works in the GitHub README, but is broken in the readthedocs documentation. Also, the link to the main folder just goes to the main page of the documentation.
  • "montain" -> "mountain" in the Reinhold and Pierrehumbert (1982) example page

We have also addressed the raised minor issues in the commit bac1d3c6 .

We thank Eviatar Bach again and hope that the proposed modifications address all the concerns.

Response to referee 2

We thank @sadielbartholomew for her thorough review. We address here her final comments, the other ones have already been addressed in the discussion above.

Thanks for opening up an issue indicating you are going to address my previous comment regarding the installation instructions, @jodemaey. I was going to re-raise that in case you missed it. Otherwise all of my feedback raised so far has been addressed well, bar one small formatting change to the text, described against point (3) below.

I have now completed the remainder of my review and I've outlined the results below (in short I'm happy with everything except for a small number of items I feel still need to be addressed). Thanks for your patience waiting for the rest of the review. Final review comments

If the following three points are addressed satisfactorily then I am more than happy to tick the remaining items in the review checklist and moreover to recommend acceptance of the qgs paper to JOSS:

  1. Regarding the state of the field , the review criterion is to 'describe how this software compares to other commonly-used packages' and whilst several similar packages are currently referenced and briefly described under a section of that name, I can't find any comparison between qgs and any of those packages in the text. There is some indication of the relation to MAOOAM, but since that is your own package and is included in qgs, I wouldn't count it as a separate package. So, what I would like to see towards fulfilling that criterion is some brief comparison between qgs and the listed alternative "easy-to-use idealized atmospheric model[s]" as you introduce them (i.e. q-gcm, pyqg, Isca). A sentence or two on what qgs offers that these libraries do not, or vice versa, would suffice. It may tie in with ground you have already covered in the first two paragraphs of the 'Statement of need' section, so you could perhaps reference them there with some further detail to address this point.

We have better described the other models in this list and have stressed the main assets of qgs in this regard. Related commits: 94188990 , 0ff4d836 and 5723bebe .

  1. The functionality of the software is documented really well, which is great. I did feel that the API reference documentation is quite hard to find, though. For example, to see the model configuration classes I would have to go through the menu like so: 'Code Description -> Modules -> Configuration of the model' and then there's a bit of introductory text before the class signature and parameters are listed, so if a reader was viewing on a laptop they might not see it and not realise the classes were covered below. In my experience of using and creating software documentation, it is most common to have the API reference as a top-level menu item, under a name such as 'API' or 'reference', where it is evident and easy to access. Therefore I advise you:
    • move the 'Modules' section out to a higher-level in the documentation tree; and/or (but ideally 'and')
    • rename it to one of the names above, as 'Modules' could imply an overview or commentary on the modules rather than the API reference for them.

We agree that the proposed change will increase the readability of the documentation. Thank you. We have implemented it in the commit 7bc75cf9.

  1. For the paper text formatting , please convert "python" to "Python" in the Performance section of the paper text (as raised in Climdyn/qgs#3 but not yet addressed).

This was done in the commit 4c1e0a85 .

Finally, I have some further comments that are not compulsory for my review acceptance , but that I think would improve the software or the paper content:

  1. I notice the labels on both the bar charts, (a) and (b), in Figure 1 are identical, so to make the visual easier to comprehend I suggest removing the labels on the top chart and moving it so the bottom is joined to the top of the lower chart, or to convert it into a single bar chart with two bars per item on the x-axis.

We do not think that this modification would make the figure easier to read. Creating a single plot could indeed be a good solution, but the data of the two actual panels are too different and that would decrease the readability of the panel (a) data.

  1. I've noticed that the repository is structured in a way that makes it difficult to search through, namely with the qgs functional modules all in separate directories at the root of the repo, instead of being contained within a directory underneath the root which is much more common. This means one can't use a simple git grep to search for keywords in the modules to aid with development, as it will pick up words from the Notebook .ipynb files (which are big JSON structures with lots of binary code to save the images etc. so clog up the terminal on a search) or the documentation source and build pages, etc., too. For that reason, so any contributors can easily work on the core code, personally I'd move all of the module directories under another dedicated directory called qgs or lib or similar.

Thank you for this suggestion. We could consider changing the structure of the repository to indeed simplify it along the line that you propose. However, we do not want to make that change now, since some consequent refactorings are planned for the v0.3. We need to think about it, and as such we have opened the issue https://github.com/Climdyn/qgs/issues/7 to discuss and come back to it later.

We thank Sadie Bartholomew again for her suggestions.

General comment

Various small language changes and consistency have also been applied. Please see the commits f8d8e41c and a318efba for a recap.

jodemaey commented 3 years ago

@whedon generate pdf from branch joss

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss. Reticulating splines etc...
whedon commented 3 years ago

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

arfon commented 3 years ago

Thanks for the detailed response @jodemaey.

@eviatarbach, @sadielbartholomew - when you get a moment, please come and take a look at the author responses and let us know what you think (updating the last few checkboxes too if appropriate).

sadielbartholomew commented 3 years ago

Thanks @jodemaey for the considered response. I have inspected your review response comment and the referenced commits and I am now satisfied that all of the review checklist items are covered well by the qgs code and the proposed paper and am delighted to recommend that the qgs paper be accepted.

Just a few closing comments, only the first of which calls for any action (ideally it could be addressed before the paper is released should everyone else is happy for it to be accepted).

I rebuilt the documentation locally to check on the restructure for the API reference and there is now a top-level 'References' section which addresses the feedback at hand, but there seems to be a setup issue since many (but not all) of the pages, notably:

all seem to be blank, whereas in the documentation currently live on qgs.readthedocs.io those pages appear to be populated with the reference information. So that should be addressed to ensure the latest documentation retains all the reference information.

We do not think that this modification would make the figure easier to read. Creating a single plot could indeed be a good solution, but the data of the two actual panels are too different and that would decrease the readability of the panel (a) data.

That is fair enough. The plots are certainly readable as they are.

Thank you for this suggestion. We could consider changing the structure of the repository to indeed simplify it along the line that you propose. However, we do not want to make that change now, since some consequent refactorings are planned for the v0.3. We need to think about it, and as such we have opened the issue Climdyn/qgs#7 to discuss and come back to it later.

I'm glad to hear you are considering this, of course address it at a time which is most appropriate in your planned roadmap.

jodemaey commented 3 years ago

Hi @sadielbartholomew ,

I rebuilt the documentation locally to check on the restructure for the API reference and there is now a top-level 'References' section which addresses the feedback at hand, but there seems to be a setup issue since many (but not all) of the pages, notably:

* Inner Products (living at `<path>/documentation/build/html/files/technical/inner_products.html`)

* Tensors module (`.../tensors.html`)

* Functions module (etc.)

* Integration modules

all seem to be blank, whereas in the documentation currently live on qgs.readthedocs.io those pages appear to be populated with the reference information. So that should be addressed to ensure the latest documentation retains all the reference information.

I cannot reproduce your problem, on my system the documentation pages that you mention are fine. Could you first perform a make clean before doing a make html again?

sadielbartholomew commented 3 years ago

Hi @jodemaey, thanks for confirming you can't recreate it. It was indeed my fault, sorry, I didn't realise but I built the local documentation last time with a different environment than the qgs conda one I used for reviewing, such that I did not have the right dependencies in my environment for Sphinx to use to parse the code to generate the API reference. I just tried it with the right environment activated and everything was there as it should be.

So everything is great now as far as I am concerned. Thanks again.

eviatarbach commented 3 years ago

The authors have addressed all my comments, and I am happy to recommend publication.

arfon commented 3 years ago

@whedon check references from branch joss