Improve control in generating foodwebs from structural models. #117

ismael-lajaaiti · 2023-02-22T14:28:32Z

Address #115 and #116.

Changes

when creating a FoodWeb a structural model the user has no access to check_cycle and check_disconnected to choose to discard or not network that respectively contains cycle(s) or disconnected species. By default, check_disconnected = true and check_cycle = false.
the maximum number of try before throwing an error for the food web generation with a structural model is now exposed to the user with the kwarg iter_safe
the error message have been corrected to include the right kwarg name (see Loosen default tolerance ΔC for large richnesses. #115)
change the default tolerance argument when generating food webs with a structural model to 10% (either of the number of links or the connectance, depending on the argument given by the user) and update in the docstring.

iago-lito · 2023-02-22T14:31:24Z

Thank you! I'm looking into this rn. But I this is not addressing #114, right? I think you meant #116 ^ ^"

ismael-lajaaiti · 2023-02-22T14:49:21Z

Thank you! I'm looking into this rn. But I this is not addressing #114, right? I think you meant #116 ^ ^"

Yes thank you, I just edited the PR message. Sorry for the confusion.

iago-lito

Thank you for this quickfix @ismael-lajaaiti :) I'll let you handle these three concerns before merging this in:

English: maybe if you're still around @Thomalpas you can be the second pair of eyes regarding docstring modifications here 0:)
I worry that the function warning_disconnected has a messy purpose. Checking is one thing, and warning is another thing. I'm sure you'd also prefer that it be split into is_disconnected and disconnection_warning or something, to ease reuse.
I worry that tol is unfortunate naming. And it's an unfortunate pattern to have it either carry a ΔC value or a ΔL value, which are not even the same type. Unless you have a good reason to keep it, I'd suggest you drop it in favour of separate ΔC and ΔL arguments.

This is it. Sorry if my comments are not entirely up to date wrt this concluding message. I write them on the fly and then I summarize all this into that ^ ^"

iago-lito · 2023-02-22T14:47:34Z

src/inputs/foodwebs.jl

@@ -111,14 +111,18 @@ true

 Moreover, while generating the `FoodWeb` we check that it does not have cycles
 or disconnected species.
+But this behavior can be changed by setting respectively the keyword arguments
+`check_cycle` and `check_disconnected` to `false`.


I'm not sure about the english here @Thomalpas?

Moreover, while generating the FoodWeb, by default we check that it does not contain cycles or disconnected species.
These behaviours can respectively be changed by setting the keyword arguments check_cycle and check_disconnected to false.

iago-lito · 2023-02-22T14:49:44Z

src/inputs/foodwebs.jl

@@ -131,6 +135,22 @@ julia> n_links(foodweb) == 15
 true
 ```

+Note that if you provide a number of `L`inks the `tol`erance is interpreted as
+as an absolute tolerance on the number of links,
+i.e. `L - tol <= n_links(foodweb) L + tol`, where `foodweb` is the output of `FoodWeb`.


Missing <= sign here maybe?

iago-lito · 2023-02-22T14:50:32Z

src/inputs/foodwebs.jl

@@ -131,6 +135,22 @@ julia> n_links(foodweb) == 15
 true
 ```

+Note that if you provide a number of `L`inks the `tol`erance is interpreted as
+as an absolute tolerance on the number of links,
+i.e. `L - tol <= n_links(foodweb) L + tol`, where `foodweb` is the output of `FoodWeb`.


"where foodweb is the output of FoodWeb".. nah you can drop that, I'm sure they'll get it ;)

iago-lito · 2023-02-22T14:59:40Z

src/inputs/foodwebs.jl

+i.e. `L - tol <= n_links(foodweb) L + tol`, where `foodweb` is the output of `FoodWeb`.
+Similarly, if you provide a `C`onnectance the `tol`erance is interpreted as
+as an absolute connectance on the connectance,
+i.e. `C - tol <= connectance(foodweb) <= C + tol`.


Hm, I sort of understand why you need to keep the tol name here because it either means ΔC or ΔL. But I keep thinking that having explicit ΔC and ΔL arguments can be helpful. Maybe you can feature both?

function f(...; tol=nothing, ΔC=nothing, ΔL=nothing) # ... s = sum(isnothing.((tol, ΔC, ΔL))) if s == 0 # setup the default values elseif s > 1 throw(ArgumentError("Only one argument among (tol, ΔC, ΔL) must be set.")) else if !isnothing(tol) # process tol elseif !isnothing(ΔC) # process ΔC elseif !isnothing(ΔL) # process ΔL end end end

or something?

UPDATE: maybe just drop tol instead.

iago-lito · 2023-02-22T15:02:01Z

src/inputs/foodwebs.jl

@@ -198,25 +218,48 @@ function FoodWeb(
    L = nothing,
    p_forbidden = nothing,
    tol = nothing,
+    check_cycle = false,
+    check_disconnected = true,
+    iter_safe = 1e5,


This scientific notation yields a floating point number. Maybe try 10^5 to get an integer instead.

iago-lito · 2023-02-22T15:05:33Z

src/inputs/foodwebs.jl

@@ -198,25 +218,48 @@ function FoodWeb(
    L = nothing,
    p_forbidden = nothing,
    tol = nothing,
+    check_cycle = false,
+    check_disconnected = true,
+    iter_safe = 1e5,


I'm not sure about the naming here. What about iter_max ?

(unless you mean something specific by "safe"?)

iago-lito · 2023-02-22T15:14:27Z

src/inputs/foodwebs.jl

    end
    (A, species, M, metabolic_class, method) = structural_foodweb_data(uni_net, M, Z, model)
+    quiet || warning_disconnected(A)


That is a pretty unusual pattern I must say. What about:

quiet || is_disconnected(A) || @warn "..."

warning_disconnected(A) does two different things: check + warn. And this makes it hard to reuse elsewhere. A simpler is_disconnected function would be more useful to the rest of the package.

Maybe you ended up with this because you didn't want to duplicate your error message. That's fair. But then you already know how to nicely factorize error messages if I trust src/macros.jl ;)

Maybe macros are overkill for the job here though, a simpler approach like:

const cycle_warning = "<blah>" function FoodWeb(...) # ... quiet || is_disconnected(A) || @warn cycle_warning end

may do the job.

Or if you need the message to be dynamically generated:

cycle_warning(a, b) = @warn "<blah> $a <blah> $b" function FoodWeb(...) # ... quiet || is_disconnected(A) || cycle_warning(a, b) end

iago-lito · 2023-02-22T15:18:26Z

src/inputs/foodwebs.jl

    C <= 1 || throw(ArgumentError("Connectance `C` should be smaller than 1."))
-    C >= (S - 1) / S^2 || throw(ArgumentError("Connectance `C` should be \
-        greater than (S-1)/S^2 ($((S-1)/S^2) for S=$S) \
+    if C < (S - 1) / S^2 && check_disconnected


Prefer

if check_disconnected && C < (S - 1) / S^2

That way, if check_disconnected is false, then julia won't even bother doing the calculation on the right operand. This is what "short-circuiting" was initially designed for ;)

iago-lito · 2023-02-22T15:23:29Z

src/inputs/foodwebs.jl

        iter += 1
    end
    iter <= iter_safe ||
        throw(ErrorException("Could not generate adequate network with C=$C \
-        and ΔC=$ΔC before the maximum number of iterations ($iter_safe) was reached. \
-        Is the constraint impossible to solve?"))
+        and tol=$ΔC before the maximum number of iterations ($iter_safe) was reached. \


Arh. With my suggestion above to feature both tol and ΔC/ΔL arguments, this error message would become difficult to get right. But I keep thinking that ΔC/ΔL is better naming than tol.

Would it be an option then to drop tol and to replace it with ΔC/ΔL completely? Is there another reason to have tol being either one or the other?

iago-lito · 2023-02-22T15:25:21Z

src/inputs/foodwebs.jl

-        and ΔC=$ΔC before the maximum number of iterations ($iter_safe) was reached. \
-        Is the constraint impossible to solve?"))
+        throw(ErrorException("Could not generate adequate network with L=$L \
+        and tol=$ΔL before the maximum number of iterations ($iter_safe) was reached. \


Yeah, and this message is ΔL-oriented while the other is only ΔC.. how come these two different values ended up in the same variable tol in the first place? Maybe this is the source of troubles here.

Thomalpas · 2023-02-22T16:13:24Z

src/inputs/foodwebs.jl

@@ -111,14 +111,18 @@ true

 Moreover, while generating the `FoodWeb` we check that it does not have cycles
 or disconnected species.
+But this behavior can be changed by setting respectively the keyword arguments
+`check_cycle` and `check_disconnected` to `false`.


Moreover, while generating the FoodWeb, by default we check that it does not contain cycles or disconnected species.
These behaviours can respectively be changed by setting the keyword arguments check_cycle and check_disconnected to false.

Thomalpas · 2023-02-22T16:14:25Z

src/inputs/foodwebs.jl

-By default, we set a tolerance of 1 link between the number of links asked by the user
-and the number of links of the returned `FoodWeb`.
+By default, we set a tolerance corresponding to 10% of the number of links
+given in argument.


given in argument L ?

"C or L"?

Thomalpas · 2023-02-22T16:15:00Z

src/inputs/foodwebs.jl

@@ -131,6 +135,22 @@ julia> n_links(foodweb) == 15
 true
 ```

+Note that if you provide a number of `L`inks the `tol`erance is interpreted as
+as an absolute tolerance on the number of links,


double 'as' ?

Thomalpas · 2023-02-22T16:19:20Z

src/inputs/foodwebs.jl

+as an absolute connectance on the connectance,
+i.e. `C - tol <= connectance(foodweb) <= C + tol`.
+
+`FoodWeb`s are generated randomly and can be rejected if do not verify


if they do not verify

note: I'm not sure about the use of the word verify, I would prefer satisfy?

Thomalpas · 2023-02-22T16:20:51Z

src/inputs/foodwebs.jl

+`FoodWeb`s are generated randomly and can be rejected if do not verify
+one of the properties defined above (number of links not in the tolerance range,
+cycles, etc.).
+Thus we repeat the generation of the `FoodWeb` until of all criterion are verified


satisfied I would prefer here too I think from an english perspective (but if there is a computational-based definition of verify then I am happy to be corrected)

I think people usually pick "satisfy" in constraints-solving contexts.

Thomalpas · 2023-02-22T16:21:31Z

src/inputs/foodwebs.jl

+Thus we repeat the generation of the `FoodWeb` until of all criterion are verified
+or until the number of maximum iteraction `iter_safe` is reached.
+If the maximum number of iterations is reached an error is thrown.
+That number can be controled by the `iter_safe` keyword argument,


controlled has 2 l's

ismael-lajaaiti · 2023-02-23T14:10:59Z

Thank you @iago-lito and @Thomalpas for you reviews! I've pushed a commit integrating your suggestions. Let me know what you think about it.

iago-lito

Great :) Thank you for the fix @ismael-lajaaiti. All my comments below are minor and easy-fix. So I'll go ahead and fix them myself before I squash/merge this in. Good job!

iago-lito · 2023-02-23T16:34:12Z

src/inputs/foodwebs.jl

@@ -6,6 +6,9 @@ Generating FoodWeb objects
 const AdjacencyMatrix = SparseMatrixCSC{Bool,Int64}
 const Label = Union{String,Symbol}

+const disconnected_warning = "'A' contains disconnected species \


Good. This is a bit far from where it's actually used, and I would reserve this file header for less anectotical definitions like abstract type EcologicalNetwork (BAM!) and mutable struct Foodweb (biim!). For example, this could live just below the FoodWeb constructor methods?

(maybe you thought it needs to be defined prior the function definition where it's used.. but it doesn't ;)

iago-lito · 2023-02-23T16:35:11Z

src/inputs/foodwebs.jl

-By default, we set a tolerance of 1 link between the number of links asked by the user
-and the number of links of the returned `FoodWeb`.
+By default, we set a tolerance corresponding to 10% of the number of links
+given in argument (`L`), or 10% of the connectance if you give a connectane (`C`).


"connectance"

iago-lito · 2023-02-23T16:35:29Z

src/inputs/foodwebs.jl

+By default, we set a tolerance corresponding to 10% of the number of links
+given in argument (`L`), or 10% of the connectance if you give a connectane (`C`).
+For instance, if you create a `FoodWeb` with 20 links,
+by default we ensure that `FoodWeb` in output will contain between 18 and 22 links.


Crystal-clear <3

iago-lito · 2023-02-23T16:36:46Z

src/inputs/foodwebs.jl

+`FoodWeb`s are generated randomly and can be rejected if they do not satisfy
+one of the properties defined above (number of links not in the tolerance range,
+cycles, etc.).
+Thus we repeat the generation of the `FoodWeb` until of all criterion are satisfied


"until all criteria"

iago-lito · 2023-02-23T16:42:43Z

src/inputs/foodwebs.jl

@@ -172,7 +193,7 @@ function FoodWeb(
    @check_size_is_richness² A S
    metabolic_class = clean_metabolic_class(metabolic_class, A)
    species = clean_labels(species, S)
-    quiet || warning_cyclic_disconnected(A)
+    quiet || is_connected(SimpleDiGraph(A)) || @warn disconnected_warning


I'm surprised you did not just add a new method for that ;)

is_connected(A::AbstractMatrix) = is_connected(SimpleDigraph(A))

Oh, scratch that: you didn't author is_connected, so that would be piracy! Good thing you didn't then ^ ^"

iago-lito · 2023-02-23T16:43:22Z

src/inputs/foodwebs.jl

@@ -197,26 +218,54 @@ function FoodWeb(
    C = nothing,
    L = nothing,
    p_forbidden = nothing,
-    tol = nothing,
+    tol_C = isnothing(C) ? nothing : 0.1 * C,
+    tol_L = isnothing(L) ? nothing : round(Integer, 0.1 * L),


Oh, I like the idea :D

iago-lito · 2023-02-23T16:44:10Z

src/inputs/foodwebs.jl

+        isnothing(tol_C) || throw(ArgumentError("You gave a number of links (`L = $L`) and \
+        a tolerance on the connectance (`tol_C = $tol_C`). \
+        Either provide a connectance (`C`) or \
+        use `tol_L` to control the tolerance on the number of links."))


iago-lito · 2023-02-23T16:47:42Z

src/inputs/foodwebs.jl

-        and ΔC=$ΔC before the maximum number of iterations ($iter_safe) was reached. \
-        Is the constraint impossible to solve?"))
+        and tol_C=$ΔC before the maximum number of iterations ($iter_max) was reached. \
+        Consider either increasing the `tol`erance on the connectance \


"the tolerance" is enough, because tol is no more.

- Loosen default values for `tol_L` and `tol_C`. - Users can control cycle checking and disconnection.

iago-lito self-requested a review February 22, 2023 14:29

iago-lito assigned ismael-lajaaiti Feb 22, 2023

This was linked to issues Feb 22, 2023

Loosen default tolerance ΔC for large richnesses. #115

Closed

Handle cycles in food webs generated by structural models. #116

Closed

iago-lito changed the title ~~Handle cycles.~~ Improve control in generating foodwebs from structural models. Feb 22, 2023

iago-lito requested changes Feb 22, 2023

View reviewed changes

Thomalpas reviewed Feb 22, 2023

View reviewed changes

iago-lito approved these changes Feb 23, 2023

View reviewed changes

Improve foodwebs generation control.

758f64e

- Loosen default values for `tol_L` and `tol_C`. - Users can control cycle checking and disconnection.

iago-lito force-pushed the default_tol branch from 7049168 to 758f64e Compare February 23, 2023 17:05

iago-lito merged commit 758f64e into BecksLab:dev Feb 23, 2023

Improve control in generating foodwebs from structural models. #117

Improve control in generating foodwebs from structural models. #117

Conversation

ismael-lajaaiti commented Feb 22, 2023 • edited Loading

Changes

iago-lito commented Feb 22, 2023

ismael-lajaaiti commented Feb 22, 2023

iago-lito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomalpas Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismael-lajaaiti commented Feb 23, 2023

iago-lito left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iago-lito Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismael-lajaaiti commented Feb 22, 2023 •

edited

Loading

Thomalpas Feb 22, 2023 •

edited

Loading

iago-lito left a comment •

edited

Loading

iago-lito Feb 23, 2023 •

edited

Loading