Skip to content
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

Add tracking to data file type column names 2. #553

Open
wants to merge 3 commits into
base: issue-511
Choose a base branch
from

Conversation

ptth222
Copy link

@ptth222 ptth222 commented Mar 17, 2024

Rebasing the original branch for this did not go well, so I manually created another one. This PR should replace #510. I was able to get this to pass all of the tests and now multiple ".* File" columns won't cause an issue. We don't need this capability like I thought we did originally, but I figured I would go on and make it work anyway.

@coveralls
Copy link

coveralls commented Mar 17, 2024

Coverage Status

coverage: 81.341% (+0.08%) from 81.257%
when pulling e50e65e on ptth222:fix-data-file-name-bug2
into 7d5f19f on ISA-tools:issue-511.

@proccaserra
Copy link
Member

as discussed today:

  • ISA Tab specification allow only 1 "Raw Data File" per Assay Table
  • "Assay Name" is needed as "Raw Data File" field can not stand alone. To ensure output "Assay Name" is output at serialization time, the protocol type should be data acquisition or a synonym has been added to the ./resources/yaml/protocol-types.yml" file
  • similarly, "Derived Data File" can not stand alone and should be associated with a "Data Transformation Name" field, which is output when serializing to ISA-Tab if a protocol type "data transformation" is found or a string declared as synonym to "data transformation" in the ./resources/yaml/protocol-types.yml" file
  • There can be more than one occurrence of "Derived Data File"
  • "Assay Name" header is the default value found associated with "Raw Data File", but 'specialization' exist:

"NMR Assay Name" with "Free Induction Decay Data File" with 'protocol type' = "NMR spectroscopy" (or declared synonyms)
"MS Assay Name" with "Raw Spectral Data File" with 'protocol type' = "mass spectrometry" (or declared synonyms)

@ptth222 ptth222 mentioned this pull request Mar 20, 2024
Changed the added test to be more realistic. Had to track name columns like Data Transformation Name so values would be handled correctly.
@ptth222
Copy link
Author

ptth222 commented Mar 21, 2024

I did an example more like what was discussed in the meeting. I had to also add tracking for the columns that are added for known protocol types. The problem that this PR solves is that for any ".* File" column, if there is more than 1 then the values don't get written out correctly. Solving this wasn't as easy as it was for a similar issue with Protocol REF columns. Due to how graph nodes were previously handled.

Old Table Output:

Sample Name Protocol REF MS Assay Name Raw Data File Protocol REF Data Transformation Name Derived Data File Protocol REF Data Transformation Name Derived Data File
sample1 protocol1 process1 datafile1.raw protocol2 process3 datafile3.raw protocol3 process3 datafile3.raw
sample2 protocol1 process4 datafile4.raw protocol3 process5 datafile5.raw process5 datafile5.raw

New Table Output:

Sample Name Protocol REF MS Assay Name Raw Data File Protocol REF Data Transformation Name Derived Data File Protocol REF Data Transformation Name Derived Data File
sample1 protocol1 process1 datafile1.raw protocol2 process2 datafile2.raw protocol3 process3 datafile3.raw
sample2 protocol1 process4 datafile4.raw protocol3 process5 datafile5.raw

@proccaserra
Copy link
Member

PRS to test this situation, i.e., several files generated by one data acquisition:

Sample Name Protocol REF MS Assay Name Raw Data File Protocol REF Data Transformation Name Derived Data File Protocol REF Data Transformation Name Derived Data File
sample1 protocol1 process1 datafile1.raw protocol2 process2 datafile2.raw protocol3 process3 datafile3.raw
sample1 protocol1 process1 datafile4.raw protocol2 process2 datafile2.raw

@@ -260,7 +263,7 @@ def get_node_by_label_and_key(labl, this_key):
fv_set.add(fv)
material.factor_values = list(fv_set)

elif object_label in _LABELS_DATA_NODES:
elif object_label in _LABELS_DATA_NODES or ' File' in object_label:
Copy link
Member

Choose a reason for hiding this comment

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

object_label="foo File" would cause issue

Copy link
Author

Choose a reason for hiding this comment

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

  1. I don't think "foo File" would validate.
  2. There are other instances of patterns like ' File' in or endswith('File') where "foo File" would cause an issue that were already present. I just made things consistent.
  3. When this change was originally made not every File column was in _LABELS_DATA_NODES.
  4. Having a list of specific acceptable file names is pretty fragile anyway and I would have generalized to columns ending in " File" a while ago, or a pattern like what "Protocol REF" does.

Copy link
Author

Choose a reason for hiding this comment

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

After discussing in meeting. I will remove this and make the code always just look in _LABELS_DATA_NODES.

def _build_paths_and_indexes(process_sequence=None):
"""Returns the paths from source/sample to end points and a mapping of sequence_identifier to object."""

def _compute_combinations(identifier_list, identifiers_to_objects):
Copy link
Member

Choose a reason for hiding this comment

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

@ptth222 please refactor to avoid nested function and revisit the nested for loops before @terazus can review.

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.

None yet

3 participants