jmadinlab / habtools

3D metrics for surfaces and objects
https://jmadinlab.github.io/habtools/
Other
12 stars 2 forks source link

error messages coming out of the fd_cubes function #40

Open jmadin opened 2 months ago

jmadin commented 2 months ago

the function corrects the lvec to make sure that largest cube divided by any value in lvec is a whole number, in doing so throws a unnecessary warning. Edit to avoid the message.

garrettfundakowski commented 2 months ago

When supplying the lvec to the function I was following the recommendations in the description - namely smallest value should not be smaller than resolution and largest value should be large enough to encapsulate the entire mesh.

As such, I created lvec like this

  # make min > res, so set max edge length instead of average
  min_cubeD <- max(vcgMeshres(mesh_loaded)[[2]])
  # make max larger than extent of the mesh
  max_cubeD <- habtools::extent(mesh_loaded) + 3 * min_cubeD
  # create a sequence of 20 values between max and min
  lvec <- seq(min_cubeD, max_cubeD, length.out = 20)

However, I was getting an error saying that "the smallest scale included in lvec is smaller than recommended". I was super confused as I explicitly supplied the minimum value to be greater than the resolution.

After some digging in the source code, I noticed that even when supplying an lvec it goes through the following code

else {
    L0 <- min(lvec)
    Lmax <- max(lvec)
    cubes <- unique(round(Lmax/lvec))
    lvec <- unique(Lmax/cubes)
  }

I think where the error was coming from specifically was in the "cubes <-..." line as the issue appeared to be coming only from cases where the resolution (my smallest value in lvec) was causing the lmax/resolution to then be rounded up. which then when you go to the next line "lvec <-..." would cause the new smallest value in lvec = Lmax/(round(Lmax/resolution)) to sometimes be smaller than the initial resolution of the mesh. Does that make sense?

My suggestion would be what if you made that floor() instead of round()? I think that would always mean the new smallest value in lvec = Lmax/(floor(Lmax/resolution)) will be greater than the initial resolution. I think that makes mathematical sense...do you agree?

*I guess this fix is for the specific case where someone defines their own lvec and sets the lowest value to be the resolution.