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

My points for the JOSS review #37

Open
nrichart opened this issue Dec 9, 2024 · 2 comments
Open

My points for the JOSS review #37

nrichart opened this issue Dec 9, 2024 · 2 comments

Comments

@nrichart
Copy link

nrichart commented Dec 9, 2024

Hello,

I am creating an issue to list the thing I noticed while testing the code for the JOSS review openjournals/joss-reviews#7525

I will list the in order of importance from my point of view from most to least important

  • The tests documented in the main README do not work for me I opened 2 specific issues Failing Test: Two-particle with wall #35 Failling Test: Compression test #36
  • The documentation does not have a Contributing section
  • There is no installation part only a build instruction, the code has to be run from the build folder
  • The generated documentation does not show properly https://prashjha.github.io/PeriDEM/
    • <a href= and @ref
    • the link for the examples do not work they point to github.io instead of to the source
    • in tools/README.md you mention dependencies that are not in the main README.md like libfann you also install more than the list of dependencies in the brew and apt commands like for example tbb
  • The build instruction cannot be copy/pasted due to the comment in the cmake command
  • There is no mention that the cmake and make command should/could be run in a build folder

--
Nicolas

@prashjha
Copy link
Owner

prashjha commented Dec 9, 2024

Hi @nrichart,

Thank you for the comments.

The failing tests are due to significant changes. I will rectify them and let you know.

  • On installation, I will see if I can modify cmake to include the install option.
  • I found it hard to work with links in doxygen. I wanted to display the same files in README.md and doxygen; I could not do this properly with doxygen. I will see if I can correct these errors as well.
  • I will address other comments also in due time.

Best,
Prashant

@nrichart
Copy link
Author

nrichart commented Dec 10, 2024

@prashjha, thanks.
For the doc the main point I think are the section titles that are also links, since this seams to generate 2 errors in the doxygen. It does not show properly but it also mess up the table of content.
The other links I understand your problem, to not have to maintain 2 places with the same content. Could full link help instead of relative ones ?
Best,
Nicolas

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

No branches or pull requests

2 participants