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

Address issue #504 #507

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dariober
Copy link
Contributor

Some imporvements probably needed

Some imporvements probably needed
@shashankbrgowda
Copy link
Contributor

@dariober Changes looks good to me and works fine.

If you don't want to use const xfeature = JSON.parse(JSON.stringify(feature)) we could consider using SimpleFeatureSerializedNoId instead of Feature as it provides getters for some of the fields.

or we can access properties like this start: "start" in feature ? feature.start as number + 1 : null

We might have to add source: gff_source and score: gff_score reserved key mapping in gffToInternal here apollo-shared/src/GFF3/gffReservedKeys.ts to avoid duplicates when we save it.

after adding mapping make necessary change in convertFeatureAttributes of gff3ToAnnotationFeature.ts.

if (existingVal) {
  if (existingVal === val) {
    continue
  }
  const valSet = new Set([...existingVal, ...val])
  convertedAttributes[newKey] = [...valSet]
}

function convertFeatureAttributes(
feature: SimpleFeatureSerializedNoId,
): Record<string, string[]> {
function convertFeatureAttributes(feature: Feature): Record<string, string[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add condition to filter attributes with undefined/null values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm missing something, it seems enough to add source and score here:

const defaultFields = new Set([
'start',
'end',
'type',
'strand',
'refName',
'subfeatures',
'derived_features',
'phase',
'source',
'score',
])

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Better to filter source and score here as they're getting added again in gff3ToAnnotationFeature.ts.

Do we also need to filter attributes with undefined/null values ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If score or source are missing, they will not be included as attribute at all. There's a test here

expect(af.attributes?.score).toBeUndefined()
expect(af.attributes?.gff_score).toBeUndefined()

If I'm not mistaken, we should not end up with attributes with null/undefined values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, tested your recent changes and its working fine. Thanks

@@ -121,6 +121,9 @@ function convertFeatureAttributes(
const newKey = isGFFReservedAttribute(key) ? gffToInternal[key] : key
const existingVal = convertedAttributes[newKey]
if (existingVal) {
// if (JSON.stringify(existingVal) === JSON.stringify(val)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

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.

2 participants