Skip to content

Fix off-by-1 indexing bug (#154) - #155

Merged
adw96 merged 1 commit into
adw96:mainfrom
mooreryan:fix-off-by-1-bug
Jun 16, 2026
Merged

Fix off-by-1 indexing bug (#154)#155
adw96 merged 1 commit into
adw96:mainfrom
mooreryan:fix-off-by-1-bug

Conversation

@mooreryan

Copy link
Copy Markdown
Contributor

Take EMburn = 3, and EMiter = 6. Originally, we're indexing from (3+1):(6+1) -> 4:7 -> c(4, 5, 6, 7). The first item is the init value. The next three c(2, 3, 4) are in the burn section, and the final three c(5, 6, 7) are in the keep section. However, we're pulling in the final one in the burn section. So it should be EMburn + 2.

See #154.


I assume the actual numerical difference made by this change would be small. To do a quick check of that I ran through the debugger using this data:

data(Lee)
Lee_phylum <- phyloseq::tax_glom(Lee, taxrank="Phylum")
divnet_phylum_char <- divnet(Lee_phylum, X = "char", tuning = "test")

Then added some diagnostic plots to the fit_atichison function. Something like this:

b0_EM:

  b0_EM <- colMeans(b0_list[(EMburn + 2):(EMiter + 1), ])

  b0_EM_old <- colMeans(b0_list[(EMburn + 1):(EMiter + 1), ])
  plot(b0_EM, b0_EM_old)
  abline(a = 0, b = 1)
b0_EM

b_EM:

    b_list_reduced <- b_list[,, (EMburn + 2):(EMiter + 1)]

    b_list_reduced_old <- b_list[,, (EMburn + 1):(EMiter + 1)]

    if (length(dim(b_list_reduced)) == 2) {
      b_list_reduced <- b_list_reduced %>% array(c(1, dim(b_list_reduced)))
      b_list_reduced_old <- b_list_reduced_old %>%
        array(c(1, dim(b_list_reduced_old)))
    }

    b_EM <- apply(b_list_reduced, c(1, 2), mean)
    b_EM_old <- apply(b_list_reduced_old, c(1, 2), mean)
    output_list$beta <- b_EM

    plot(as.vector(b_EM), as.vector(b_EM_old))
    abline(a = 0, b = 1)
b_EM

sigma_em:

  sigma_em <- apply(sigma_list[,, (EMburn + 2):(EMiter + 1)], c(1, 2), mean)

  sigma_em_old <- apply(sigma_list[,, (EMburn + 1):(EMiter + 1)], c(1, 2), mean)
  plot(as.vector(sigma_em), as.vector(sigma_em_old))
  abline(a = 0, b = 1)
sigma_em

(If you look at the actual values there very close.)

Take EMburn = 3, and EMiter = 6. Originally, we're indexing from
(3+1):(6+1) -> 4:7 -> c(4, 5, 6, 7). The first item is the init value.
The next three c(2, 3, 4) are in the burn section, and the final three
c(5, 6, 7) are in the keep section. However, we're pulling in the final
one in the burn section.  So it should be EMburn + 2.

See adw96#154.
@adw96
adw96 self-requested a review June 11, 2026 20:38

@adw96 adw96 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much for this fix, @mooreryan

@adw96
adw96 merged commit 57fbdc2 into adw96:main Jun 16, 2026
3 checks passed
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