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

Refactor functioning.jl. #79

Closed
ismael-lajaaiti opened this issue Nov 30, 2022 · 15 comments · Fixed by #87
Closed

Refactor functioning.jl. #79

ismael-lajaaiti opened this issue Nov 30, 2022 · 15 comments · Fixed by #87
Assignees

Comments

@ismael-lajaaiti
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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

@ismael-lajaaiti
Copy link
Collaborator Author

ismael-lajaaiti commented Dec 7, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

(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
Copy link
Contributor

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
Copy link
Collaborator Author

ismael-lajaaiti commented Dec 12, 2022

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
Copy link
Contributor

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

@iago-lito
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator Author

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

@alaindanet
Copy link
Contributor

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

@alaindanet
Copy link
Contributor

Now in #87 🎆

@iago-lito iago-lito linked a pull request Jan 13, 2023 that will close this issue
@iago-lito iago-lito removed a link to a pull request Jan 13, 2023
@iago-lito iago-lito linked a pull request Jan 13, 2023 that will close this issue
@iago-lito
Copy link
Collaborator

Closed by #87.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants