Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address JOSS review comments #394

Merged
merged 7 commits into from
Jul 23, 2024
Merged

Address JOSS review comments #394

merged 7 commits into from
Jul 23, 2024

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Jul 22, 2024

PR Summary

In the the JOSS review #391 @parikshitbajpai points out two issues. The first is that the examples directory is not documented. The second is that the paper seems to blend the statement of need and state of the field sections. I address both comments here:

For openjournals/joss-reviews#6805

  • For the exampled directory, I add a page to the documentation explaining how to build and run the examples, as well as what they do. I also noticed a few problems, which I resolve:
    • The customizing page of the docs was not properly indexed and was not showing up in HTML builds.
    • The CMake for the examples folder was actually commented out in the build system. This must have been an intermediate debugging change that somehow made it into main.
    • The example that gets sound speed and pressure must not have been built in a while because it showed usage of the deprecated EOSBuilder machinery, which no longer exists.
  • For the paper, I merge the two sections and simply call the joint section statement of need and state of the field.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.

@Yurlungur Yurlungur requested review from junghans and jhp-lanl July 22, 2024 23:02
@Yurlungur Yurlungur self-assigned this Jul 22, 2024
@Yurlungur Yurlungur added bug Something isn't working documentation Improvements or additions to documentation and removed bug Something isn't working labels Jul 22, 2024
@Yurlungur Yurlungur linked an issue Jul 22, 2024 that may be closed by this pull request
2 tasks
@Yurlungur Yurlungur mentioned this pull request Jul 22, 2024
2 tasks
Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typos.

I skimmed the actual HTML documentation build and it looks good, but please give it a check of your own to ensure it looks as you intend it to look
https://lanl.github.io/singularity-eos/jmm/document-examples/src/examples.html#examples

doc/sphinx/src/building.rst Outdated Show resolved Hide resolved
doc/sphinx/src/examples.rst Show resolved Hide resolved
Copy link
Contributor

@parikshitbajpai parikshitbajpai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me and addresses both my comments in #391. A very minor suggestion to fix the directory name but otherwise it looks great.

doc/sphinx/src/examples.rst Outdated Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator Author

Minor typos.

I skimmed the actual HTML documentation build and it looks good, but please give it a check of your own to ensure it looks as you intend it to look https://lanl.github.io/singularity-eos/jmm/document-examples/src/examples.html#examples

Yep. I actually renewed my practice of building the html locally. This helped me find a bunch of problems I fixed here. :)

@Yurlungur Yurlungur merged commit 03ccf13 into main Jul 23, 2024
5 checks passed
@Yurlungur Yurlungur deleted the jmm/document-examples branch July 23, 2024 14:53
@junghans
Copy link
Member

That looked good to me as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JOSS-Review
5 participants