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

First use cases. #107

Merged
merged 1 commit into from
Feb 28, 2023
Merged

First use cases. #107

merged 1 commit into from
Feb 28, 2023

Conversation

ismael-lajaaiti
Copy link
Collaborator

@ismael-lajaaiti ismael-lajaaiti commented Feb 13, 2023

Here is my first use case (reproduction of results from Miele et al. 2019). You can add yours by rebasing your work on this branch and pushing your commits.
Note that I've created an utils.jl script that contains utility functions that could be use in the different use case scripts, in particular there is a function to save plots. If advise use to also use this function to save your plot(s) and obviously you can modify it, if wanted.
I'll push my second use case here soon.

@ismael-lajaaiti ismael-lajaaiti requested review from alaindanet and iago-lito and removed request for alaindanet February 13, 2023 17:40
@iago-lito iago-lito added the documentation Improvements or additions to documentation label Feb 14, 2023
@iago-lito iago-lito linked an issue Feb 14, 2023 that may be closed by this pull request
5 tasks
Copy link
Collaborator

@iago-lito iago-lito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ismael-lajaaiti and thank you for this first full-fleshed use-case :)

Nitpicks aside, here is are the major points to address yet:

  • Autoformatting checks do not pass yet under use_cases/ folder.

  • The way you use MultiplexNetwork() with that get_kwargs() helper function is waay too complicated, and we cannot expect our users to be able to do so on their own. Use something closer from the API instead (suggestion in the comments).

  • I fail to see why save_plot() is necessary, and why it needs to record three different file formats on disk. Are you intending to automatically integrate these plots into some sort of documentation later, within the scope of this repo?

  • I would be happy to have this script be run as part of our test suite. It's long, so we would need to separate it from the other, short tests. I can handle that if you need. However:

    • Having this script running without bugs is a good "smoke test" that latest modifications have not broken anything.. but maybe you can also think of something more "actionable" to verify that the diversity results are conform to some expectations?

I hear that you've just pushed a new commit to this branch. Lemme have a look at it now :)

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
use_cases/utils.jl Outdated Show resolved Hide resolved
use_cases/utils.jl Outdated Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Outdated Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Outdated Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Outdated Show resolved Hide resolved
push!(plot_vec, p)
end
plot(plot_vec...)
save_plot("nti-intensity-vs-diversity")
Copy link
Collaborator

@iago-lito iago-lito Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not sure why it's necessary to have all three formats saved at the same time then, or to have them even saved on disk at all :\

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just in case, as I said above. Also I was thinking that this function could contain some parameters (e.g. the figure size) that we want to keep consistent through the different scripts within the use_case/ subdirectory.

Copy link
Collaborator

@iago-lito iago-lito Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion: saving figures on disk is out-of-scope for use_cases/, so we won't bother with this.

Copy link
Collaborator

@iago-lito iago-lito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have almost nothing but nitpicks to add with your latest commit and the new use-case.

The only thing is that I have a growing concern about use_cases/utils.jl. I worry that the scope of this file is ill-defined. It contains either functions that are very generic and which I think should rather be defined within the package and exported from it, OR functions that are very specific and which I think serve no exact purpose in the context of use_cases.

Maybe I have misunderstood what use_cases/ is for. Here is what I think it is (tick if you agree):

  • Scripts under use_cases/ use EcologicalNetworksDynamics.jl to produce the interesting example outcomes discussed in the associated paper.
  • Scripts under use_cases/ produce the figures used in the paper.
  • Scripts under use_cases/ should be reproducible, and they should not break with future updates of the package. As such, they are meant to be executed by developers as a form of testing.
    • If yes, then I can help you with that.
  • Scripts under use_cases/ serve as example usage of the package. As such, they are meant to be read by users as a form of documentation.
    • If yes, then I worry that helper functions such that save_plot() (essentially a wrapper around savefig()) distract them from the main message here, i.e. "here is how you use the package, and you can save your plots with regular savefig". The same goes for save_data() (essentially a wrapper around serialize()).

use_cases/.JuliaFormatter.toml Outdated Show resolved Hide resolved
use_cases/utils.jl Outdated Show resolved Hide resolved
use_cases/utils.jl Outdated Show resolved Hide resolved
use_cases/utils.jl Outdated Show resolved Hide resolved
use_cases/utils.jl Outdated Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
@ismael-lajaaiti
Copy link
Collaborator Author

Here is the WIP commit that integrate most of your suggestions @iago-lito, some still need to be discussed before being integrated.

Copy link
Collaborator

@iago-lito iago-lito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ismael-lajaaiti, I'm finding no major concern anymore here, although I think some previous comments/questions have not been addressed yet. Can you get through them again so we make sure we're not forgetting anything?

In general, I'm not exactly sure how these example scripts should be read: are they supposed to be some sort of tutorials?
If yes, then I'm afraid the core loops are a little bit crude for the readers. Had I been unaware of EcologicalNetworksDynamics.jl and then given that double-loop here, I'd be like "wow, how am I supposed to write this on my own?" Better documenting (commenting) them a bit more.
If no, then I guess we could get along with them, and expect the readers to go check the documentation for more information.

Besides, I think that biomass-vs-mortality.jl would be a good fit for the boost, and a good benchmark/test for the future. So why not boosting it? I also hear from #118 that reducing use cases execution time would actually be a good thing 0;)

.gitignore Outdated Show resolved Hide resolved
src/measures/structure.jl Show resolved Hide resolved
src/measures/structure.jl Show resolved Hide resolved
src/measures/structure.jl Outdated Show resolved Hide resolved
src/measures/structure.jl Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Outdated Show resolved Hide resolved
use_cases/diversity-vs-nti-intensity.jl Outdated Show resolved Hide resolved
@iago-lito iago-lito changed the title Use cases First use cases. Feb 24, 2023
@ismael-lajaaiti
Copy link
Collaborator Author

Hi @ismael-lajaaiti, I'm finding no major concern anymore here, although I think some previous comments/questions have not been addressed yet. Can you get through them again so we make sure we're not forgetting anything?

In general, I'm not exactly sure how these example scripts should be read: are they supposed to be some sort of tutorials? If yes, then I'm afraid the core loops are a little bit crude for the readers. Had I been unaware of EcologicalNetworksDynamics.jl and then given that double-loop here, I'd be like "wow, how am I supposed to write this on my own?" Better documenting (commenting) them a bit more. If no, then I guess we could get along with them, and expect the readers to go check the documentation for more information.

Besides, I think that biomass-vs-mortality.jl would be a good fit for the boost, and a good benchmark/test for the future. So why not boosting it? I also hear from #118 that reducing use cases execution time would actually be a good thing 0;)

Hello @iago-lito thanks for your review 😊
I've gone through your older comments, now they should been all address, sorry for my lack of attention.
Use cases are supposed to be advanced tutorial. So I assume that the reader has gone (at least quickly) through the core documentation and know the basics of the package. However, I agree that I could have more gentle with the reader so I added few more comments (also note that my idea was to encapsulate this script in a markdown file, that would be incorporated in the documentation, so in the end I intended to a bit of text to motivate and explain what these scripts does).
I boosted biomass-vs-mortality.jl 🚀

Copy link
Collaborator

@iago-lito iago-lito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ismael-lajaaiti, and good job with this last round :) I am happy with it now, and am okay to address the minor comments below myself. I'll just direct-contact you to get a few answers and then merge this in!

src/measures/structure.jl Show resolved Hide resolved
src/measures/structure.jl Outdated Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Show resolved Hide resolved
use_cases/biomass-vs-mortality.jl Outdated Show resolved Hide resolved
@iago-lito iago-lito force-pushed the use_cases branch 2 times, most recently from 7de0fa3 to 0429153 Compare February 28, 2023 09:27
- Mortality.
- Non-trophic interations intensity *vs.* diversity.
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.

Add use cases to the documentation
3 participants