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

Display gene subfeatures even if topLevelFeatures is not set #1340

Merged
merged 2 commits into from Mar 19, 2019

Conversation

Projects
None yet
3 participants
@abretaud
Copy link
Contributor

abretaud commented Mar 18, 2019

This PR allows to display gene subfeatures in HTMLFeatures (and NeatHTMLFeature) tracks, even if topLevelFeatures is not set "properly".

This is typically for gff files containing gene > mRNA > exon/CDS data.

Setting topLevelFeatures works nice, but :

  • people uploading gff files don't know about it, and they have no easy way to set it from the upload box (and in the end they complain about it to me)
  • CanvasFeatures tracks are already able to display this kind of data with all the subfeatures
  • this is such a typical gff structure that I would expect jbrowse to display it properly out of the box

The code is inspired/adapted from #996

Without this patch:
subfeats_bad

With this patch:
subfeats_ok

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Mar 18, 2019

I am open to other opinions, but to me this changes a default behavior that jbrowse has had for basically forever, and even though it is basically better this way, I guess I would say this alternative behavior needs to be dependent on a configurable global variable that an instance admin can set (e.g. browser.config.inferHTMLSubfeatures or similar)

@nathandunn nathandunn self-requested a review Mar 18, 2019

@nathandunn
Copy link
Contributor

nathandunn left a comment

I think this is correct.

I think that @cmdcolin point is valid, however, I think if this is the default/ desired behavior, maybe we leave it in as-is. I ended up working around this by automatically specifying it, but I agree that this is the behavior a user would expect 98% of the time. We could add a global to ignore the other 2%

@abretaud

This comment has been minimized.

Copy link
Contributor Author

abretaud commented Mar 19, 2019

Yes I think it's a much better default behavior like this for most use cases. Let me know if I should add a global option for this in this PR

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Mar 19, 2019

Yes please make it an optional flag

@abretaud

This comment has been minimized.

Copy link
Contributor Author

abretaud commented Mar 19, 2019

Ok, done, It's enabled by default, is it ok or not?

@nathandunn

This comment has been minimized.

Copy link
Contributor

nathandunn commented Mar 19, 2019

I think this is a sane default, but I defer to @cmdcolin if he thinks it might break something.

abretaud added a commit to abretaud/jbrowse that referenced this pull request Mar 19, 2019

@nathandunn nathandunn merged commit fce2f21 into GMOD:dev Mar 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cmdcolin cmdcolin added this to the 1.16.4 milestone Mar 19, 2019

abretaud added a commit to abretaud/jbrowse that referenced this pull request Mar 22, 2019

@abretaud abretaud referenced this pull request Apr 11, 2019

Merged

JBrowse: 1.16.4 #2365

3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.