Skip to content

Illumina binning is indexed by ASCII instead of Phred. - #68

Merged
mvassilev merged 6 commits into
developfrom
Issue67
Aug 12, 2026
Merged

Illumina binning is indexed by ASCII instead of Phred.#68
mvassilev merged 6 commits into
developfrom
Issue67

Conversation

@mvassilev

Copy link
Copy Markdown
Collaborator

Fixes #67.

@codecov-commenter

codecov-commenter commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71.83%. Comparing base (7fda4d8) to head (3424037).

Files with missing lines Patch % Lines
src/rntuple/RAMNTupleRecord.cxx 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #68      +/-   ##
===========================================
+ Coverage    71.66%   71.83%   +0.17%     
===========================================
  Files           19       19              
  Lines         1969     1985      +16     
  Branches       779      776       -3     
===========================================
+ Hits          1411     1426      +15     
+ Misses         443      427      -16     
- Partials       115      132      +17     
Flag Coverage Δ
unittests 71.83% <96.77%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
test/ramcoretests.cxx 100.00% <100.00%> (ø)
src/rntuple/RAMNTupleRecord.cxx 68.03% <93.33%> (+0.14%) ⬆️

... and 1 file with indirect coverage changes

Files with missing lines Coverage Δ
test/ramcoretests.cxx 100.00% <100.00%> (ø)
src/rntuple/RAMNTupleRecord.cxx 68.03% <93.33%> (+0.14%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/rntuple/RAMNTupleRecord.cxx Outdated
// SAM stores quality as Phred+33 ASCII, but kIlluminaBinning is
// indexed by the Phred VALUE. Without the -33 every lookup lands 33
// slots too far right
int phred = static_cast<unsigned char>(qual[i]) - 33;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: variable 'phred' is not initialized [cppcoreguidelines-init-variables]

Suggested change
int phred = static_cast<unsigned char>(qual[i]) - 33;
int phred = 0 = static_cast<unsigned char>(qual[i]) - 33;

Comment thread src/rntuple/RAMNTupleRecord.cxx Outdated
// slots too far right
int phred = static_cast<unsigned char>(qual[i]) - 33;
if (phred < 0)
phred = 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: use std::max instead of < [readability-use-std-min-max]

Suggested change
phred = 0;
phred = std::max(phred, 0);

Comment thread src/rntuple/RAMNTupleRecord.cxx Outdated
if (phred < 0)
phred = 0;
if (phred > 93)
phred = 93; // SAM's maximum; also keeps us inside the initialised table

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: use std::min instead of > [readability-use-std-min-max]

Suggested change
phred = 93; // SAM's maximum; also keeps us inside the initialised table
phred = std::min(phred, 93); // SAM's maximum; also keeps us inside the initialised table

Comment thread test/ramcoretests.cxx Outdated
auto roundTrip = [&](int q) {
binRecord.SetQUAL(phred(q));
const std::string out = binRecord.GetQUAL();
EXPECT_EQ(out.size(), 2u);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: integer literal has suffix 'u', which is not uppercase [readability-uppercase-literal-suffix]

Suggested change
EXPECT_EQ(out.size(), 2u);
EXPECT_EQ(out.size(), 2U);

Comment thread test/ramcoretests.cxx Outdated
return static_cast<int>(static_cast<unsigned char>(out[0])) - 33;
};

struct {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

   struct { int in; int want; } kBins[] = {
   ^

Comment thread test/ramcoretests.cxx
RAMNTupleRecord losslessRecord;
losslessRecord.SetQUAL("*");
EXPECT_EQ(losslessRecord.GetQUAL(), "*");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: integer literal has suffix 'u', which is not uppercase [readability-uppercase-literal-suffix]

Suggested change
EXPECT_EQ(binRecord.GetQUAL().size(), 10U);

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread test/ramcoretests.cxx Outdated
return static_cast<int>(static_cast<unsigned char>(out[0])) - 33;
};

struct {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

   struct {
   ^

Comment thread test/ramcoretests.cxx Outdated

// Length must be preserved for real quality strings.
binRecord.SetQUAL("IIIIIIIIII");
EXPECT_EQ(binRecord.GetQUAL().size(), 10u);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: integer literal has suffix 'u', which is not uppercase [readability-uppercase-literal-suffix]

Suggested change
EXPECT_EQ(binRecord.GetQUAL().size(), 10u);
EXPECT_EQ(binRecord.GetQUAL().size(), 10U);

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// index inside the initialised part of the table (entries 110..255
// are zero-filled, so an out-of-range value would silently decode as
// Q0 -- the opposite error, but still an error).
const int phred = std::clamp(static_cast<int>(static_cast<unsigned char>(qual[i])) - 33, 0, 93);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: variable 'phred' is not initialized [cppcoreguidelines-init-variables]

Suggested change
const int phred = std::clamp(static_cast<int>(static_cast<unsigned char>(qual[i])) - 33, 0, 93);
const int phred = 0 = std::clamp(static_cast<int>(static_cast<unsigned char>(qual[i])) - 33, 0, 93);

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/rntuple/RAMNTupleRecord.cxx Outdated
// index inside the initialised part of the table (entries 110..255
// are zero-filled, so an out-of-range value would silently decode as
// Q0 -- the opposite error, but still an error).
const int phred = 0 = std::clamp(static_cast<int>(static_cast<unsigned char>(qual[i])) - 33, 0, 93);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

warning: variable 'phred' is not initialized [cppcoreguidelines-init-variables]

Suggested change
const int phred = 0 = std::clamp(static_cast<int>(static_cast<unsigned char>(qual[i])) - 33, 0, 93);
const int phred = 0 = 0 = std::clamp(static_cast<int>(static_cast<unsigned char>(qual[i])) - 33, 0, 93);

@mvassilev
mvassilev merged commit 85e419a into develop Aug 12, 2026
5 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.

Illumina binning is indexed by ASCII instead of Phred

2 participants