BecksLab / EcologicalNetworksDynamics.jl

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

Refactor `functioning.jl`. #79

Closed ismael-lajaaiti closed 1 year ago

ismael-lajaaiti commented 1 year ago

Refactoring the functioning.jl IMO some part of this script can be re-written to make it more clear/short and also some part are not consistent with the rest of the package (use of return, docstring not formatted exactly as advise by the Julia official documentation, etc.)

iago-lito commented 1 year ago

Yupe. What is the purpose/scope of this file btw? Is it clearly distinct from the measures eventually implemented in the other measures package?

ismael-lajaaiti commented 1 year ago

Yes the metrics written in this file are different from the one implemented in the package of stability metrics (if it is the one you talk about). Actually, the metrics written in this file are more related to the network structure than to its stability.

iago-lito commented 1 year ago

I see, well, better renaming the file to something containing at least the keyword measure or metric then?

ismael-lajaaiti commented 1 year ago

Something like structure_metrics? @alaindanet what do you think? Does it seem good to you? Note that there is already another file named structure, maybe you could merge the two files. However, the metrics defined in the functioning are more oriented toward analyzing the network at the of the simulation (for the user), while metrics within structure are mainly used internally (by us developers) even if some of them can be useful for the users (e.g. trophic_levels). So one could argue that these two scripts achieve two slightly different goals, even if the frontier is blurry.

iago-lito commented 1 year ago

He-hee, blurry indeed :) Well, I'd advise to base your choice on this very kind semantic considerations (before vs. after simulation, user vs. dev utils, ..), but also acknowledge that this is unstable yet and that the files may be renamed/merged/split in the future : so don't overthink this and keep it simple and modular so as to ease future changes 0:)

iago-lito commented 1 year ago

(wops, sorry I'm just realizing that functioning.jl is actually src/measures/functioning.jl, which makes it very clear that it contains "measures". Sorry for the noise :(

alaindanet commented 1 year ago

structure_metrics sounds good to me! Do you want me to refactor src/measures/functioning.jl ? It is an old code from me and I think that I can improve it.

I was also thinking about returning species biomasses in the total_biomass() , added to the return of total biomass. May be also also that all those functions should have a common prefix for consistency, such as foodweb_, so total_biomass() could be renamed as foodweb_biomass() ?

What do you think?

ismael-lajaaiti commented 1 year ago

If it's ok for you maybe it's best that you do the first refactoring pass, and then, Iago and me can do a second pass by reviewing your new code. Does it seem good to you?

For the naming, do you mean that you want that all analysis functions (working on the output of simulate()) to have a common prefix? If so, I'm not sure this is necessary, as long as their name states clearly what they do and that they are well documented. But maybe @iago-lito has a different view on that? Also foodweb is maybe not the best prefix as these methods would also work on MultiplexNetworks, something like analysis could be better?? Lastly, I'm really not sure about that, but here modules could be an alternative, as I think they would allow to call analysis functions with Analysis.[function_name] (but again I'm not sure this is necessary/useful).

alaindanet commented 1 year ago

I agree with you, may be there is no need for such prefix!

iago-lito commented 1 year ago

I don't have a particular opinion regarding naming functions in this file because I'm not exactly confident what they mean, what the user shall invoke them for, or what semantic unity makes them all live within the same file. I'll trust you on this one guys :)

What I can approve, however, is that the temptation to "prefix all functions with `something_`"* is a strong sign that you want namespaces. In Julia modules are the effective way to namespace things, according to the following pattern:


# Instead of:
general_A() = ...
general_B() = ...
species_B() = ...
species_C() = ...

# Prefer

A() = ...
B() = ...

module Species
    B() = ..
    C() = ..
end

# That you use with:
A()
B()
Species.B()
Species.C()

.. or something. This is very in accordance with the last item on the Zen of Python :P

However, keep in mind the 5th item (higher-rank, higher priority): Flat is better than nested. So be careful not to end up with a complicated 3-layered modules structure that becomes a nightmare to navigate and maintain. Use just the right amount of them ;)

alaindanet commented 1 year ago

I refactored all the metrics (function naming, arguments, doc), everything should be cleaner now.

I might ask what should be on the Christmas list (a bit late) now for us as output measures!

ismael-lajaaiti commented 1 year ago

Great! I'm sorry I'm a bit lost, on which branch are your changes (so I can have a quick look :innocent:)? And is the branch pushed?

alaindanet commented 1 year ago

The branch is pushed and is this one: https://github.com/BecksLab/BEFWM2/tree/simulation_output_utils

alaindanet commented 1 year ago

Now in #87 :fireworks:

iago-lito commented 1 year ago

Closed by #87.