-
Notifications
You must be signed in to change notification settings - Fork 3
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
First use cases. #107
Conversation
There was a problem hiding this 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 thatget_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?
- 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
I hear that you've just pushed a new commit to this branch. Lemme have a look at it now :)
push!(plot_vec, p) | ||
end | ||
plot(plot_vec...) | ||
save_plot("nti-intensity-vs-diversity") |
There was a problem hiding this comment.
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 :\
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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 export
ed 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/
useEcologicalNetworksDynamics.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 aroundsavefig()
) distract them from the main message here, i.e. "here is how you use the package, and you can save your plots with regularsavefig
". The same goes forsave_data()
(essentially a wrapper aroundserialize()
).
- If yes, then I worry that helper functions such that
Here is the WIP commit that integrate most of your suggestions @iago-lito, some still need to be discussed before being integrated. |
75f7bbc
to
7091dce
Compare
7091dce
to
682c5ca
Compare
There was a problem hiding this 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;)
682c5ca
to
e73ecad
Compare
Hello @iago-lito thanks for your review 😊 |
There was a problem hiding this 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!
7de0fa3
to
0429153
Compare
- Mortality. - Non-trophic interations intensity *vs.* diversity.
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.