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

Index properties #895

Closed
wants to merge 6 commits into from
Closed

Index properties #895

wants to merge 6 commits into from

Conversation

vmikk
Copy link

@vmikk vmikk commented Dec 15, 2023

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

@shawnlaffan
Copy link
Owner

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

shawnlaffan commented Dec 15, 2023

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

The bug is in here:

my $bounds
= $self->{indices}{$index}{bounds}
//= $self->get_index_is_nonnegative($index) ? [0,'Inf']
: $self->get_index_is_unit_interval($index) ? [0,1]
: $self->get_index_is_categorical($index) ? []
: ['-Inf','Inf'];

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

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

@shawnlaffan shawnlaffan mentioned this pull request Dec 17, 2023
1 task
@shawnlaffan
Copy link
Owner

@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.

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 this pull request may close these issues.

2 participants