Skip to content

Conversation

@knapii-developments
Copy link
Contributor

The transform function did not take into account r formatted files. An index/out of range error would've occurred due to the length header being one less than the other rows.

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #127 into master will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage   52.35%   52.57%   +0.21%     
==========================================
  Files          22       22              
  Lines        2611     2623      +12     
==========================================
+ Hits         1367     1379      +12     
  Misses       1244     1244              
Impacted Files Coverage Δ
ingest/expression_files/dense_ingestor.py 96.45% <100.00%> (+0.32%) ⬆️
ingest/expression_files/expression_files.py 81.57% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d713a04...db2ddc5. Read the comment docs.

@knapii-developments knapii-developments marked this pull request as ready for review August 13, 2020 18:59
data_arrays = []
for all_cell_model in self.set_data_array_cells(self.header[1:], ObjectId()):
# Cell names are formatted differently in R files
cell_names = self.header if self.is_r_file else self.header[1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

Like this enhancement - it makes filter_expression_scores much more regular.

Copy link
Contributor

@jlchang jlchang left a comment

Choose a reason for hiding this comment

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

Requesting minor change to test_filter_expression_scores. Otherwise looks good.

float(expression_score)
):
valid_expression_scores.append(expression_score)
# add one to account for gene name in scores list
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is now out of date and should be removed now that cell_names is being passed in instead of self.header for cells.

Comment on lines 36 to 42
scores = ["BRCA1", 4, 0, 3, "0", None, "", " ", "nan", "NaN", "Nan", "NAN"]
cells = ["foo", "foo2", "foo3", "foo4", "foo5", "foo6", "foo7"]
cells = ["foo", "foo2", "foo3", "foo4", "foo5"]
actual_filtered_values, actual_filtered_cells = DenseIngestor.filter_expression_scores(
scores[1:], cells
)
self.assertEqual([4, 3], actual_filtered_values)
self.assertEqual(["foo2", "foo4"], actual_filtered_cells)
self.assertEqual(["foo", "foo3"], actual_filtered_cells)
Copy link
Contributor

Choose a reason for hiding this comment

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

The test here should have another valid value after all the values that are expected to be filtered to demonstrate that filtered values and cells names do not get "out of sync" during filtering because of the non-numeric NaN values.

Copy link
Contributor

@devonbush devonbush left a comment

Choose a reason for hiding this comment

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

Approve pending response to Jean's comments

@knapii-developments knapii-developments merged commit 9b90193 into master Aug 17, 2020
@knapii-developments knapii-developments deleted the ea-r-validations branch October 16, 2020 12:35
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.

5 participants