Fix smasher failing to include all samples in large studies #2365
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cgreene
reviewed
Jun 30, 2020
| for sample_page in ( | ||
| samples[i * page_size : i + page_size] for i in range(0, len(samples), page_size) | ||
| ): | ||
| for sample_page in (samples[i : i + page_size] for i in range(0, len(samples), page_size)): |
There was a problem hiding this comment.
I don't love using i here since i usually think of i as a loop counter (it looks like whoever implemented this first did as well since they multiplied it by page size). Could we use start?
cgreene
reviewed
Jun 30, 2020
Co-authored-by: Casey Greene <cgreene@users.noreply.github.com>
|
This also looks like it would affect quantpendias. I think that between this and #2368 we should probably rerun quantpendia. |
|
Yeah I agree. The original issue mentioned specifically our mouse and human quantpendia missing samples. |
|
strong agree on a re-run |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
#2211
Purpose/Implementation Notes
It looks like an indexing issue was preventing us from copying over any more than the first 100 quant files for each experiment in the smasher. Since the range's third parameter is
page_size, on the second iterationiispage_size, so we effectively takesamples[page_size ** 2 : page_size * 2]which will be empty.Methods
If this pull request has any implications for data or metadata processing or addresses an issue labeled
sci review, please include an overview of the methods used (e.g., briefly explain how the data gets processed).See #267 for rationale.
Include sufficient detail for reviewers or users that are not expert developers to evaluate the validity of the approach.
Please attach or link to example input and output data if applicable.
It may also be appropriate to include a description of any functional or unit tests in this section depending on their content.
Any pull request with a methods section requires scientific review in addition to code review.
Types of changes
What types of changes does your code introduce?
Functional tests
List out the functional tests you've completed to verify your changes work locally.
Checklist
Put an
xin the boxes that apply.Screenshots
Please attach any screenshots that illustrate these changes.