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

Englishing docstrings ✏️ #125

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Conversation

Thomalpas
Copy link
Collaborator

I've finally had a read of the docstrings @alaindanet wrote for the simulation output utils and made a few minor changes.

There are 4 things that I think are worth discussing rather than me just changing them:

  1. Within the richness docstring (line 15) within functioning.jl we have:
  • threshold: biomass threshold below which a species is considered extinct. Set to 0 by
    default and it is recommended to leave as this. It is recommended to change the threshold
    using ExtinctionCallback in simulate.

Which reads as both we should leave the threshold and change it... I'm not sure what information the second sentence is meant to be providing so I don't know how to change it

  1. There are some inconsistences with solution::Solution vs solution when writing function arguments - for example:
    function richness(solution::Solution; vs function species_persistence(solution;

Is there a purpose to this or should this be tidied up to always solution::Solution?

  1. Line 435 within functioning.jl reads docstring = """. Everywhere else this is """

I have left this because I have assumed this is deliberate and beyond my knowledge, but just wanted to check this was meant to be there?

  1. Maybe you've had this discussion in the hundreds of comments on the original pull request but have we considered replacing instances of "alive" and "living" within function names with "extant"?

@iago-lito
Copy link
Collaborator

Hi @Thomalpas and thank you for this again :) I'll answer what I can and leave the rest to discuss with @alaindanet:

2. There are some inconsistences with solution::Solution vs solution

Indeed. The default is to just use function f(solution). But sometimes it's useful that a function has two methods that work differently depending on the argument type, like function f(solution::Solution) and function f(matrix::AbstractMatrix). In some cases, it is necessary to specify the argument type explicitly so there is no ambiguity for Julia.

However, having one general-purpose function f(solution) and one specialized version function f(matrix::AbstractMatrix) would definitely work in this case, and contribute to making the docs simpler and more consistent. Do think there are still ::Solution in your code that could be elided without loss of functionality @alaindanet?

3. Line 435 within functioning.jl reads docstring = """

Indeed. This very docstring is special because it is bound to automatically generated functions (here). We need to store it in a variable first so we can use it in a loop later :)

4. [..] have we considered replacing instances of "alive" and "living" within function names with "extant"

We have not, and it could make sense indeed. Any thought @alaindanet, @ismael-lajaaiti, @andbeck?

@iago-lito iago-lito added the documentation Improvements or additions to documentation label May 5, 2023
@iago-lito
Copy link
Collaborator

(follow-up of your comment there: if we go for indices instead of indexes, but the abbreviated parameters names are still idxs, is that a problem? Or do we need to rename them to inds or something?)

@Thomalpas
Copy link
Collaborator Author

(follow-up of your comment there: if we go for indices instead of indexes, but the abbreviated parameters names are still idxs, is that a problem? Or do we need to rename them to inds or something?)

I don't have a problem personally with idxs - it has been used by others e.g. https://discourse.julialang.org/t/differentialequations-jl-solution-object-save-idxs-and-interpolation/54030

(I think in my head it is because idxs is plural of idx and therefore is fine)

@ismael-lajaaiti
Copy link
Collaborator

ismael-lajaaiti commented May 8, 2023

  1. [..] have we considered replacing instances of "alive" and "living" within function names with "extant"

We have not, and it could make sense indeed. Any thought @alaindanet, @ismael-lajaaiti, @andbeck?

I agree that it makes sense. I think we should go for it.

@iago-lito
Copy link
Collaborator

Triage: blocked until output simulation utils eventually make it to the new API.

@iago-lito iago-lito force-pushed the englishing_simulation_outputs branch from a846405 to 1692040 Compare March 15, 2024 14:59
@iago-lito iago-lito marked this pull request as draft March 19, 2024 19:40
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.

3 participants