Review PR [do not merge] #8
Conversation
…recent changes to code base
Cannon edits, with resolved merged conflicts
… it doesnt totally work so commented out)
…needs work on mismatched input and output failures/edit on repeated sampels (aka only not allow repeated items).
…ariable names for clarity
Error checking
Demo bug fixes
Update rows and columns
Fix plate orientation
Made edits and added to the row_col
There was a problem hiding this comment.
@cashmonger @malloc3
Both CollectionManagement/CollectionActions and CollectionDisplay seem like they should be in Krill.
There are also potentially classes that we might want to make -- for instance input and output arrays.
Also need to find a way to avoid exposing field values, or at least don't use that name.
more to say, but lets go back and forth on this
| # @param operations [OperationList] operation list | ||
| # @param role [String] whether object is an input or an output | ||
| # @param location [String] the location to put things | ||
| def table_of_job_object_location(operations, role: 'input', location: nil) |
There was a problem hiding this comment.
This method name doesn't say what is being done.
Also, this is a basic operation and should be in Krill. In fact, there is already code that puts things away. If it doesn't do what is needed, then it should be changed.
There was a problem hiding this comment.
As a rule, if you are mentioning field values, it is probably not protocol code and needs to be moved into Krill.
There was a problem hiding this comment.
@bjkeller this looks like an Aquarium issue.
| # | ||
| # @param operations [OperationList] the list of operations | ||
| # @param location [String] the location that the items are to be moved to | ||
| def store_input_collections(operations, location: nil) |
There was a problem hiding this comment.
This method is not unique to collections, and doesn't really belong here. See comment on table_of_job_object_location
Also, you shouldn't have methods with basically the same show block. Factor out the commonality with store_output_collections.
| # | ||
| # @param array_of_fv [Array] array of field values | ||
| # @return obj_array [Array] array of objects (either collections or items) | ||
| def get_obj_from_fv_array(array_of_fv) |
| # that extends class Item | ||
| # @param location [String] the location to move object to | ||
| # (String or Wizard if Wizard exists) | ||
| def set_locations(obj_array, location) |
There was a problem hiding this comment.
not unique to collections.
also, probably could be Krill
There was a problem hiding this comment.
@bjkeller this looks like an Aquarium issue.
|
|
||
| # Instructions to store a specific collection | ||
| # | ||
| # @param collection [Collection] the collection that is to be put away |
There was a problem hiding this comment.
name doesn't match parameter
There was a problem hiding this comment.
This has aspects that are not unique to collections. I'm not clear on how this can be generalized, but should look at it.
| # Parts are copied from the WorkflowValidation Lib see note below for explination | ||
|
|
||
| def precondition(_op) | ||
| range = (50...100) |
There was a problem hiding this comment.
is it possible to do this with units?
| # @param range [Range] acceptable numerical range of concentration | ||
| def valid_conc?(op, range) | ||
| op.input_array("Input Array").each do |field_value| | ||
| conc = field_value.part.get("Stock Conc (ng/ul)".to_sym) |
There was a problem hiding this comment.
this should be abstracted.
RNA_Seq/OperationTypes/RNA_QC.rb
Outdated
| note 'Save output data as CSV and upload on next page' | ||
| end | ||
|
|
||
| csv_uploads = get_validated_uploads(working_plate.parts.length, |
There was a problem hiding this comment.
Can this be library or Krill code?
There was a problem hiding this comment.
@bjkeller this looks like an Aquarium issue.
RNA_Seq/OperationTypes/RNA_QC.rb
Outdated
| # TODO write highlight heat map method for table | ||
| # | ||
| # @param working_plate [Collection] the plate being used | ||
| def list_concentrations(working_plate) |
There was a problem hiding this comment.
concentrations seems like another thing that could be a library (similar to QC)
StandardLibs/UploadHelper.rb
Outdated
| end | ||
| end | ||
|
|
||
| # Needs documentation |
There was a problem hiding this comment.
mark these with TODO: – many editors will flag them
… while being more general, additional changes reflected within protocols and other libraries to support.
…cols, Front end looks mostly the same however tracking and managing of samples is notable improved and generalized
Provenance fix
…ns to libraries that will be cascaded through the rest of the protocols. Need to finish testing and confirm everything is working properly before the rest of the protocols get the updates
Major updates
this is for code review