Skip to content

Commit

Permalink
Dropped uqc QC type for the review check
Browse files Browse the repository at this point in the history
  • Loading branch information
@mgcam
mgcam committed Oct 15, 2024
1 parent 3eab96d commit 8a2e632
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 202 deletions.
19 changes: 12 additions & 7 deletions Changes
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
LIST OF CHANGES FOR NPG-QC PACKAGE

- Previously the code allowed for an empty applicability_criteria hash, which
resulted in a particular set of QC criteria being applied to every and any
product. Very early on this was an intended behaviour for UKB data. The main
filter was the study id. The default section of the product configuration
file does not have an external filter, so there is a real danger of the
review check being run indiscriminately for any product. While this will
never be an intention, small errors in the YML file might have this effect.
- npg_qc::autoqc::check::review:
1. Previously the code allowed for an empty applicability_criteria hash,
which resulted in a particular set of QC criteria being applied to every
and any product. Very early on this was an intended behaviour for UKB
data. The main filter was the study id. The default section of the product
configuration file does not have an external filter, so there is a real
danger of the review check being run indiscriminately for any product.
While this will never be an intention, small errors in the YML file might
have this effect.
2. Ability to set UQC outcomes was introduced for the Heron project. This
functionality was never used and is unlikely to be used in future, it is
now removed.

release 72.2.1 (2024-10-04)
- Added .github/dependabot.yml file to auto-update GitHub actions
Expand Down
1 change: 0 additions & 1 deletion MANIFEST
View file
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,6 @@ t/data/autoqc/review/mqc_type/product_release.yml
t/data/autoqc/review/lims_applicability_empty/product_release.yml
t/data/autoqc/review/no_known_applicability_type/product_release.yml
t/data/autoqc/review/unknown_qc_type/product_release.yml
t/data/autoqc/review/uqc_type/product_release.yml
t/data/autoqc/review/default_and_study_section/product_release.yml
t/data/autoqc/review/default_section/product_release.yml
t/data/autoqc/review/wrong_default_and_study_section/product_release.yml
Expand Down
30 changes: 9 additions & 21 deletions lib/npg_qc/autoqc/checks/review.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Readonly::Array my @APPLICABILITY_CRITERIA_TYPES => (
Readonly::Scalar my $ACCEPTANCE_CRITERIA_KEY => q[acceptance_criteria];

Readonly::Scalar my $QC_TYPE_DEFAULT => q[mqc];
Readonly::Array my @VALID_QC_TYPES => ($QC_TYPE_DEFAULT, q[uqc]);
Readonly::Array my @VALID_QC_TYPES => ($QC_TYPE_DEFAULT);

Readonly::Scalar my $TIMESTAMP_FORMAT_WOFFSET => q[%Y-%m-%dT%T%z];

Expand Down Expand Up @@ -106,11 +106,6 @@ marked as 'Preliminary' (examples: 'Accepted Final',
'Rejected Preliminary'). By default the final_qc_outcome flag is
false and the produced outcomes are preliminary.
A valid User QC outcome is one of the values from the
uqc_outcome_dict table of the npg_qc database. A concept of
the finality and, hence, immutability of the outcome is not
applicable to user QC outcome.
The type of QC outcome can be configured within the Robo QC
section of product configuration. The default type is library
Manual QC.
Expand Down Expand Up @@ -332,8 +327,9 @@ sub execute {
}

$self->result->criteria($self->_criteria);
my $md5 = $self->result->generate_checksum4data($self->result->criteria);
$self->result->criteria_md5($md5);
$self->result->criteria_md5(
$self->result->generate_checksum4data($self->result->criteria)
);
my $err;

try {
Expand All @@ -344,7 +340,7 @@ sub execute {
$self->result->add_comment($err);
};
not $err and $self->result->qc_outcome(
$self->generate_qc_outcome($self->_outcome_type(), $md5));
$self->generate_qc_outcome($self->_outcome_type()));

return;
}
Expand All @@ -371,34 +367,26 @@ sub evaluate {
Returns a hash reference representing the QC outcome.
my $u_outcome = $r->generate_qc_outcome('uqc', $md5);
my $m_outcome = $r->generate_qc_outcome('mqc');
=cut

sub generate_qc_outcome {
my ($self, $outcome_type, $md5) = @_;
my ($self, $outcome_type) = @_;

$outcome_type or croak 'outcome type should be defined';

my $package_name = 'npg_qc::Schema::Mqc::OutcomeDict';
my $pass = $self->result->pass;
#####
# Any of Accepted, Rejected, Undecided outcomes can be returned here
my $outcome = ($outcome_type eq $QC_TYPE_DEFAULT)
? $package_name->generate_short_description(
$self->final_qc_outcome ? 1 : 0, $pass)
: $package_name->generate_short_description_prefix($pass);
my $outcome = $package_name->generate_short_description(
$self->final_qc_outcome ? 1 : 0, $pass);

$outcome_type .= '_outcome';
my $outcome_info = { $outcome_type => $outcome,
timestamp => create_current_timestamp(),
username => $ROBO_KEY};
if ($outcome_type =~ /\Auqc/xms) {
my @r = ($ROBO_KEY, $VERSION);
$md5 and push @r, $md5;
$outcome_info->{'rationale'} = join q[ ], @r;
}

return $outcome_info;
}
Expand Down
124 changes: 3 additions & 121 deletions t/50-schema-result-Review.t
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use strict;
use warnings;
use Test::More tests => 6;
use Test::More tests => 5;
use Test::Exception;
use Moose::Meta::Class;
use Digest::MD5 qw/md5_hex/;
Expand Down Expand Up @@ -306,135 +306,17 @@ subtest 'a full insert/update record with mqc outcome' => sub {
is ($rs->next->description, 'Accepted final', 'outcome has not changed');
};

subtest 'a full insert/update record with uqc outcome' => sub {
plan tests => 36;

my $initial_num_records = $schema->resultset($table)->search({})->count();

my $uqc_table = 'UqcOutcomeEnt';
my $id_seq_composition = t::autoqc_util::find_or_save_composition(
$schema, {'id_run' => 1111,
'position' => 1,
'tag_index' => 33});

my $uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition});
is ($uqc_rs->count, 0, 'no uqc records for this entity');
my $rs = $schema->resultset($table)->search({id_seq_composition => $id_seq_composition});
is ($rs->count, 0, 'no review records for this entity');

my $qc_outcome = {"uqc_outcome" => "Rejected",
"timestamp" => "2018-06-03T12:53:46+0000",
"username" => "robo_qc",
"rationale" => "testrobo"};
my $values = {
id_seq_composition => $id_seq_composition,
library_type => 'common_type',
evaluation_results => {"e1"=>1,"e2"=>0},
criteria => {"and"=>["e1","e2"]},
qc_outcome => $qc_outcome,
pass => 0,
path => 't/data'
};
my $cmd5 = md5_hex(JSON::XS->new()->canonical(1)->encode($values->{criteria}));

isa_ok($schema->resultset($table)->create($values), 'npg_qc::Schema::Result::' . $table);
is ($schema->resultset($table)->search({})->count(), $initial_num_records + 1,
'total number of records in teh review table went up by one');
$rs = $schema->resultset($table)->search({id_seq_composition => $id_seq_composition});
is ($rs->count, 1, 'one row created in the review table');
my $row = $rs->next;
is_deeply ($row->qc_outcome, $qc_outcome, 'qc outcome saved');
is_deeply ($row->evaluation_results, {"e1"=>1,"e2"=>0}, 'evaluation results saved');
is_deeply ($row->criteria, {"and"=>["e1","e2"]}, 'criteria saved');
is ($row->criteria_md5, $cmd5, 'checksum saved');
$uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition});
is ($uqc_rs->count, 1, 'one row created in the uqc table');
my $outcome = $uqc_rs->next;
is ($outcome->rationale, 'testrobo', 'rationale recorded');
is ($outcome->description, 'Rejected', 'correct uqc outcome');
is ($outcome->modified_by, $ENV{USER}, 'correct user');
is ($outcome->username, 'robo_qc', 'correct user');
my $dt = $outcome->last_modified();
is ($dt->year, 2018, 'correct year');
is ($dt->month, '6', 'correct month');
is ($dt->minute, '53', 'correct minute');
is ($dt->second, '46', 'correct second');

$qc_outcome = {"uqc_outcome" => "Accepted",
"timestamp" => "2018-08-05T12:53:46+0000",
"username" => "robo_qc",
"rationale" => "testrobo1"};
$values = {
id_seq_composition => $id_seq_composition,
library_type => 'common_type',
evaluation_results => {"e1"=>1,"e2"=>1},
criteria => {"and"=>["e1","e2"]},
qc_outcome => $qc_outcome,
};
$row->update($values);
is_deeply ($row->evaluation_results, {"e1"=>1,"e2"=>1}, 'evaluation results updated');
is_deeply ($row->qc_outcome, $qc_outcome, 'qc outcome updated');
$outcome = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition})
->next;
is ($outcome->rationale, 'testrobo1', 'rationale updated');
is ($outcome->description, 'Accepted', 'uqc outcome updated');
is ($outcome->last_modified()->month, 8, 'month updated');
is ($outcome->last_modified()->year, 2018, 'correct year');

$id_seq_composition = t::autoqc_util::find_or_save_composition(
$schema, {'id_run' => 1111,
'position' => 1,
'tag_index' => 34});
$uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition});
is ($uqc_rs->count, 0, 'no uqc records for this entity');
my $new_uqc = $schema->resultset($uqc_table)
->new_result({id_seq_composition => $id_seq_composition});
$new_uqc->update_outcome({uqc_outcome => 'Accepted', rationale => 'test1'}, 'user1', 'test');
$uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition});
is ($uqc_rs->count, 1, 'record created');

$values = {
id_seq_composition => $id_seq_composition,
library_type => 'common_type',
evaluation_results => '{"e1":1,"e2":1}',
criteria => '{"and":["e1","e2"]}',
qc_outcome => {"uqc_outcome" => "Undecided",
"timestamp" => "2018-09-03T12:58:43+0000",
"username" => "robo_qc",
"rationale" => "testrobo2"},
pass => undef
};
my $created=$schema->resultset($table)->create($values);

is ($schema->resultset($table)->search({})->count(), $initial_num_records + 2,
'total number of records in teh review table went up by one');
$uqc_rs = $schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition});
is ($uqc_rs->count, 1, 'one uqc record for the entity');
$outcome = $uqc_rs->next;
is ($outcome->rationale, 'testrobo2', 'rationale updated');
is ($outcome->description, 'Undecided', 'outcome updated');
is ($outcome->modified_by, $ENV{USER}, 'correct user');
is ($outcome->username, 'robo_qc', 'correct user');
$dt = $outcome->last_modified();
is ($dt->year, 2018, 'correct year');
is ($dt->month, '9', 'correct month');
is ($dt->minute, '58', 'correct minute');
is ($dt->second, '43', 'correct second');
};


subtest 'unknown outcome type should give an error' => sub {
plan tests => 5;

my $uqc_table = 'UqcOutcomeEnt';
my $id_seq_composition = t::autoqc_util::find_or_save_composition(
$schema, {'id_run' => 1111,
'position' => 1,
'tag_index' => 43});

my $no_records = sub {
is ($schema->resultset($uqc_table)->search({id_seq_composition => $id_seq_composition})->count,
0, 'no uqc records for this entity');
is ($schema->resultset($mqc_table)->search({id_seq_composition => $id_seq_composition})->count,
0, 'no mqc records for this entity');
is ($schema->resultset($table)->search({id_seq_composition => $id_seq_composition})->count,
0, 'no review records for this entity');
};
Expand Down
16 changes: 2 additions & 14 deletions t/60-autoqc-checks-review.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ subtest 'single expression evaluation' => sub {
};

subtest 'evaluation within the execute method' => sub {
plan tests => 48;
plan tests => 40;

local $ENV{NPG_CACHED_SAMPLESHEET_FILE} =
't/data/autoqc/review/samplesheet_29524.csv';
Expand All @@ -436,7 +436,6 @@ subtest 'evaluation within the execute method' => sub {
is ($o->result->pass, undef, 'result pass attribute is unset');

my @check_objects = ();

push @check_objects, npg_qc::autoqc::checks::review->new(
runfolder_path => $rf_path,
conf_path => $test_data_dir,
Expand All @@ -446,12 +445,7 @@ subtest 'evaluation within the execute method' => sub {
conf_path => "$test_data_dir/mqc_type",
qc_in => $dir,
rpt_list => $rpt_list);
push @check_objects, npg_qc::autoqc::checks::review->new(
conf_path => "$test_data_dir/uqc_type",
qc_in => $dir,
rpt_list => $rpt_list);

my $count = 0;
foreach my $check (@check_objects) {
lives_ok { $check->execute } 'execute method runs OK';
is ($check->result->pass, 1, 'result pass attribute is set to 1');
Expand All @@ -461,15 +455,9 @@ subtest 'evaluation within the execute method' => sub {
is_deeply ($check->result->evaluation_results(), \%expected,
'evaluation results are saved');
my $outcome = $check->result->qc_outcome;
if ($count < 2) {
is ($outcome->{'mqc_outcome'} , 'Accepted preliminary', 'correct outcome string');
} elsif ($count == 2) {
is ($outcome->{'uqc_outcome'} , 'Accepted', 'correct outcome string');
ok ($outcome->{'rationale'} , 'rationale is set');
}
is ($outcome->{'mqc_outcome'} , 'Accepted preliminary', 'correct outcome string');
is ($outcome->{'username'}, 'robo_qc', 'correct process id');
ok ($outcome->{'timestamp'}, 'timestamp saved');
$count++;
}

# Undefined library type should not be a problem.
Expand Down
38 changes: 0 additions & 38 deletions t/data/autoqc/review/uqc_type/product_release.yml
View file

This file was deleted.

0 comments on commit 8a2e632

Please sign in to comment.