BecksLab / EcologicalNetworksDynamics.jl

A simulator for ecological dynamics written in Julia.
GNU Affero General Public License v3.0
18 stars 1 forks source link

Finalize use cases for submission. #119

Closed alaindanet closed 1 year ago

alaindanet commented 1 year ago

I put my usecase in the dedicated folder following @ismael-lajaaiti usecases!

Happy to have your feedback on it!

ismael-lajaaiti commented 1 year ago

Here is my review @alaindanet. Maybe before diving into it wait for @iago-lito's round that will add a layer of comments on top of mine.

Before summarizing my comments, this is really good and I am mainly nitpicking, thank you!

Below are the main points I raised in my comments:

ismael-lajaaiti commented 1 year ago

Here are my first modifications. I integrated the above suggestions and while re-writing stuff I found that the two loops (loop 1: generation of trophic networks, loop 2: add producer competition and run simulations) could be merged. I also now use a DataFrame to store the output and I removed the unnecessary information that was saved in the output. Also instead of using println to show simulations progress I now use the @info macro.

Coming next: use Makie and maybe AlgebraOfGraphics for the plot.

EDIT: I am adding below the to do list of the remaining things I aim to address this PR

ismael-lajaaiti commented 1 year ago

Coming next: use Makie and maybe AlgebraOfGraphics for the plot.

Here it is 🚀! I really like the Makie experience, the code for the plot is a bit longer than before, but I find it more intuitive and more customizable. Also I used the theme set_aog_theme!() from AlgebraOfGraphics, so we can ensure a consistent style between the different figures with one function (and this theme is quite nice).

@iago-lito you can go for your review 🤓

ismael-lajaaiti commented 1 year ago

I've reviewed and added Hana's use case. I've also added the part of code to plot the figure with the theme used for the other use case figures. I still need to add some comments to clarify some parameter choices, I'll update it as soon as I have answers from Hana.

ismael-lajaaiti commented 1 year ago

I am quite happy with the result, use cases are now all consistent in terms of code style and of the figures they produce. I think I've done all I could done for the moment. I am ready for your review @iago-lito.

The next step would is to incorporate these use case in the documentation, instead of having them as raw Julia scripts in a separate folder. If I have time I may tackle this before next week.

EDIT: I can't integrate them in the documentation at least for the NTI use case, because of #118 😬. I think one trick could be to slightly modify the use case so that it runs faster (e.g. decrease community size). I think it is not an issue if the use case in the online documentation do not perfectly corresponds to the one in the paper. As we can still keep the scripts to produce the figures of the paper in the use_case/, there the code should remain untouched, or rather we should commit ourselves to maintain this code so it always produce the same output (in case we introduce breaking changes). But we can let ourselves some flexibility for the online due to the computational constraints. IMO this is the best compromise.

ismael-lajaaiti commented 1 year ago

Thanks again @iago-lito for your review and bringing up the data race issue and all your other suggestions. I really have the feeling to improve a lot thanks to them 🙏. I am going to rerun the use cases to ensure that they all run correctly. Once it is done, I will push my corrections. 🚀

ismael-lajaaiti commented 1 year ago

Here are my corrections. Also it seems that I forgot to previously push the paradox_enrichment.jl use case, so you haven't seen this script yet. Still I tried to adapt the suggestions you made for the other use cases in it. Let me know, what you think about it. 😉

ismael-lajaaiti commented 1 year ago

Thank you :) No worries, I'll wait #87 to be merged. I incorporated your latest suggestions.

To see the plot see my comment above.

If not working from an IDE, CairoMakie won't display plots contrary to the Plots package. You need to save the figure to see it. Try:

save("path/to/save/the/figure", fig; resolution = (450, 350), px_per_unit = 3)

The kwargs are optional, but ensure you that the figure will look very nice. 😆

iago-lito commented 1 year ago

87 Has landed! I'll rebase this on top of that and land it asap :)