search

shawnlaffan / biodiverse

A tool for the spatial analysis of diversity
http://shawnlaffan.github.io/biodiverse/
GNU General Public License v3.0
73 stars 19 forks source link

Index properties #895

Closed vmikk closed 7 months ago

vmikk commented 7 months ago

This PR introduces modifications to properties of certain indices, specifically focusing on bounds and distribution. (related with #894)

shawnlaffan commented 7 months ago

Thanks for the PR.

Are these indices missing the bounds in the JSON structure? If so then there is a bug elsewhere in the code as any index flagged as unit_interval should have bounds of [0,1] automatically added by the get_index_bounds call when the metadata object is created.

The system should also be hierarchical so if an index has no distribution then it inherits the one from the calculation. This makes it easier to set if a calculation has several indices, all with the same distribution. As with the previous case, the values should be assigned to each index when the metadata object is created.

shawnlaffan commented 7 months ago

And now I see some of the indices need tweaking.

The PHYLO_RW_TURNOVER_A, B and C indices are the sums of branch lengths so can be any value. I considered setting them to non_negative but it is possible to have trees with negative branch lengths. Such lengths would make a mess of the index so perhaps we should put a constraint on the trees used. (Update - this is slated under #896).

ENDW_CWE_NORM is set to unit_interval but its bounds are incorrect. That's a bug in the code.

shawnlaffan commented 7 months ago

The bug is in here: https://github.com/shawnlaffan/biodiverse/blob/f46918598f653868a05e12585aada37b113da4d4/lib/Biodiverse/Metadata/Indices.pm#L144-L149

Unit intervals are non_negative so are all being assigned the non-Negative bounds. The order of conditions needs to change to check for unit_interval first.

shawnlaffan commented 7 months ago

Initial work and some optimisations are on the index_bounds2 branch: https://github.com/shawnlaffan/biodiverse/tree/index_bounds2

shawnlaffan commented 7 months ago

@vmikk - I just merged #897 which should fix the underlying issues that motivated this PR. It also makes the JSON file more readable.

Can you try again with the master branch?

I'll close this PR now. However, if there are remaining issues with the bounds (likely) then a new PR would be welcome. Or raise an issue first to discuss implementation options.