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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify plate and well specifications for sparse plates #24
Clarify plate and well specifications for sparse plates #24
Changes from 3 commits
8b24328
0c28690
5a1ddc7
f20c03c
7c2536a
3c31c14
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding #24 (comment), are there cases where it is not possible to recompute these indexes based on the knowledge of the individual wells
path
as well as therows
andnames
dictionaries? If recomputing is always possible (but at the cost of the consumer), my primary consideration is whether the recommendation for these new fields should beSHOULD
rather thanMUST
.For real-world examples, I can definitely see how
row_index/column_index
makes sense in terms of optimizing some of the queries. In addition to testing this with sparse plates, it will be useful to also generate representative plate with many wells (384 at least) to check there is no performance impact with the extra JSON metadata.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for these indexes to be forward or reverse computable,
path
would need to be much more explicitly defined than it is now:Furthermore, the
wells
array would need have be null or similar padding in order for those indexes to make sense.Neither of these things are ideal obviously. I don't think there's a way to not have these things be
MUST
if we want to guarantee that lookups can happen based on physical plate characteristics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise that #70 has been added after this PR was opened, but the decision there means these new attributes should now be named
rowIndex
andcolumnIndex
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed in 7c2536a.