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 review coments from @munkm #646

Open
MicahGale opened this issue Jan 27, 2025 · 2 comments · May be fixed by #647
Open

Address review coments from @munkm #646

MicahGale opened this issue Jan 27, 2025 · 2 comments · May be fixed by #647
Assignees
Labels
documentation Improvements or additions to documentation PyOpenSci / JOSS
Milestone

Comments

@MicahGale
Copy link
Collaborator

MicahGale commented Jan 27, 2025

@munkm has graciously provided a review as part of the PyOpenSci peer-review process here: pyOpenSci/software-submission#205 (comment).

I have copied the actionable comments below:

  • It looks to me in your badges that you do include a test coverage and a docs deploy badge, but not a test status badge (passing/not passing). I do see your test results from the most recent merged PR.
  • You may want to include install instructions for optional dependencies for interested users/developers or some language in your documentation that to run some examples you will need extra dependencies. For example, you have a .ipynb dile in /demos/ but you do not list jupyter notebooks as an optional dependency. I see that in your pyproject.toml file you have a demos configuration, but this isn't documented in your installation instructions that I saw.
  • To build off of @jpmorgan98 's comment about PyNE features, I think adding a reference to PyNE to the paper would be good too.
  • In the paper you mention MCNPtools, but do not cite it. The MCNPtools repo does have a preferred citation: https://github.com/lanl/mcnptools
  • In reviewing the examples and documentation I didn't see documentation on how to set boundary conditions (I did see the is_reflecting property and is_white_boundary for surfaces, as well as periodic_surface. You do a great job of describing how to bring surfaces together to form cells, but an example on setting these would be helpful.
  • In reviewing the examples I had a question: in the example above Setting and Modifying Geometry in the User Guide you specify building a cylindrical geometry using AxisPlane to form two truncating planes. You don't use any default orientation for these in the example. Is it assumed to be x on writing (is there a default?) ? Or is there a requirement to specify a plane before writing? I think a note to document this and the preferred method to set it would be helpful.
  • I had one last thought @MicahGale -- I know the scope of this project is to support the full MCNP manual eventually. If you do plan to support tallies and eigenvalue configurations (I may have missed it if you said it either way?), you may want to include something in your API documentation and in the README that there is a path for these in the future.

In addition the following boxes weren't checked:

  • Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
    • Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
@MicahGale MicahGale added documentation Improvements or additions to documentation PyOpenSci / JOSS labels Jan 27, 2025
@MicahGale MicahGale self-assigned this Jan 27, 2025
@MicahGale MicahGale linked a pull request Jan 28, 2025 that will close this issue
10 tasks
@MicahGale MicahGale linked a pull request Jan 28, 2025 that will close this issue
10 tasks
@MicahGale
Copy link
Collaborator Author

Google SEO is saying none of our pages are canonical because of /en so adding a canonical meta is important: https://developers.google.com/search/docs/crawling-indexing/consolidate-duplicate-urls?visit_id=638737140017289164-120786721&rd=1

@MicahGale
Copy link
Collaborator Author

This is a better guide on canonical URLs: https://docs.readthedocs.com/platform/stable/canonical-urls.html

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 PyOpenSci / JOSS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant