Skip to content

Update returns#116

Merged
Zilong-Li merged 7 commits intorwdavies:masterfrom
robert-vogel:update_returns
Jan 19, 2026
Merged

Update returns#116
Zilong-Li merged 7 commits intorwdavies:masterfrom
robert-vogel:update_returns

Conversation

@robert-vogel
Copy link
Contributor

I noticed a compiler error / warning when a build the package from source and then run R CMD check STITCH_1.8.4.tar.gz:

  • phasing.cpp: return type void despite being declared int. The for loop has a conditional statement that if it never evaluates to true the function will continue until the terminal } and consequently return void. I just added an error integer.
  • vcfpp.h: the htslib/vcf.h macro bcf_int32_missing is an integer and I think should be used in a conditional statement instead of as a function macro.

Hope it helps.

…n phasing.cpp indicated a return type of int, but there was a path through the function that returns void. I defined an error macro value and return that integer. I also received an error that htslib macro bcf_int32_missing (defined in htslib/vcf.h) was used incorrectly, I think it is supposed to be used to test equality.
…n phasing.cpp indicated a return type of int, but there was a path through the function that returns void. I defined an error macro value and return that integer. I also received an error that htslib macro bcf_int32_missing (defined in htslib/vcf.h) was used incorrectly, I think it is supposed to be used to test equality.
@robert-vogel
Copy link
Contributor Author

I am trying to figure out why the update didn't pass unit tests. Stay tuned for an update.

…int is assigned to a single output block. The trouble was at the boundary at the buffered and not buffered regions. A set of SNPs in a grid point could lie in the buffer region, which should be excluded. I updated the unit test to reflict this. Moreover, I found the function determine_snp_and_grid_blocks_to_output very difficult to read. It was a combination of the excessive use of parenthesis, spacing, and some variable name. I updated and added more documentation to help the next person reviewing this code to have an easier time understanding its purpose and design.
@robert-vogel
Copy link
Contributor Author

Hi @Zilong-Li the unit tests were failing due to a bug in the unit-test code. The problem was that a grid point spans SNP output buffer regions at the boundary of SNP in SNP output blocks. Consequently, the SNPs assigned to one grid point may have value NA and whichever grid index, causing the unit test to fail.

The changes I made and disclosed above did not seem to create the unit test from failing. I still recommend these changes.

I also checked if there was a bug in the STITCH function determine_snp_and_grid_blocks_for_output in the STITCH/R/function.R file. The code as a difficult to follow as there was no documentation of the function to explain its purpose, some of the variables names were unhelpful, parentheses were overused, and spacing made the code visually challenging. I updated these in the main code base to help me figure out if what was being done. I didn't find a bug, but the update may be useful for someone else reading.

@Zilong-Li Zilong-Li merged commit 786d591 into rwdavies:master Jan 19, 2026
6 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