Skip to content

New gwas track - #503

Open
lldelisle wants to merge 11 commits into
deeptools:developfrom
lldelisle:new_gwas_track
Open

New gwas track#503
lldelisle wants to merge 11 commits into
deeptools:developfrom
lldelisle:new_gwas_track

Conversation

@lldelisle

Copy link
Copy Markdown
Collaborator

This is based on #493

@lldelisle

Copy link
Copy Markdown
Collaborator Author

@bgruening would you mind to review this PR?

@lldelisle
lldelisle requested a review from bgruening September 2, 2026 20:14
line = next(self.file_handle)
line = to_string(line)
if line.startswith("#") or line.strip() == '':
line = self.get_no_comment_line()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really need a recursion here? Will a file with a lot of comments hit the python recursion stack?

   for line in self.file_handle:
       line = to_string(line)
       self.line_number += 1
       if not (line.startswith("#") or line.strip() == ''):
           return line

simple forloop?

from .utilities import InputError, to_string


class ReadGwas(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe refactor to ReadTabular or something like that and share with GTF, BED etc?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes I will do this

from .utilities import InputError, to_string


class ReadGwas(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This (object) thing is also something we should clean for version 4.0 ... its old Python2 syntax afaik.

# Fill in the position and pvalues lists with data from the GWAS file
position = [region.begin for region in gwas_overlap]
# Notice the -log10 transformation
y_values = [-np.log10(region.data.pvalue) if region.data.pvalue > 0 else 0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mh, a pvalue of 0 is plotted at Y=0 ... is that what we want?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@AnttonLA can you justify your choice?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fair comment. The clamping is just to avoid breaking the log function, but this then causes invalid p-values (negative or 0) to be plotted at Y=0.
They could be removed instead, or an error could be raised. Depends on how you want to handle incorrect input data

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Could we set the transformation as a parameter? like we do in bedgraph or bigwig?
Currently, options are:

  • transform: no (default) or log, log1p, -log, log2 or log10.
  • log_pseudocount: 0 (default) or any float

And we could add -log10

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If p-values are 0 or negative it would raise an error, see:

def transform(score_list, transform, log_pseudocount, file):

show_data_range = true
# the default for min_value and max_value is 'auto' which means that the scale will go
# roughly from the minimum value found in the region plotted to the maximum value found.
min_value = 0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isn't the detault auto?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The default is auto. We put here suggestions for possible values.

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.

3 participants