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

fix gap_fraction_profile and LAD for one layer profile #719

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

floriandeboissieu
Copy link
Contributor

@floriandeboissieu floriandeboissieu commented Oct 12, 2023

Hi Jean-Romain,
In commit Fix LAD if not computable you added a few lines to return a 0 row data.frame when there is two layers or less for LAD.

I do not see the reason for that: a unique layer with points inside (and eventually below) should still give a gap fraction and thus a LAD, no?

In that pull-request, I made the changes to do so: in LAD() I only returned a 0 row data.frame when gap_fraction_profile was returning a 0 row data.frame.

By the way, I simplified several things in gap_fraction_profile(), with the gap fraction being the gf=N_out/N_in:

  • no need to compute gap fraction until min(z) if min(z)<z0 --> starting from z0 and considering the points below in a unique layer [-Inf, z0]
  • the only case I see that seq() returns one value, i.e. (length(brk)==1) is when z0==max(z), thus I changed the test.
  • dividing the counts by sum(counts) is canceled afterwards by shift(cs)/cs, thus I removed that first division
  • the supplementary layer with 0 counts does not bring anything (actually it is removed in the end) thus I removed it
  • i can be computed without the need of data.table::shift (unless it is much faster then 1:length(cs)?), thus I removed that dependency here.
  • if i=NaN, is.na(i) is TRUE thus i would be set to 0 before getting to is.nan(i) --> I removed it as I do not see any reason for a NaN to happen here (it would be the case of i=0/0, which would never happen I think). And as a consequence gap_fraction_profile() will never return an NA value thus I removed the anyNA test in LAD()
  • I removed the test [z > z0] at the end as I do not see any reason for it

I hope I did not missed anything.

@Jean-Romain Jean-Romain merged commit dd16f69 into r-lidar:master Oct 27, 2023
2 checks passed
@Jean-Romain
Copy link
Collaborator

Sorry I forgot about that one. It sounds good thank you.

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