Skip to content

[query] Fix old hl.plot.histogram log argument#11268

Merged
danking merged 2 commits into
hail-is:mainfrom
johnc1231:johnc-fix-old-plot-hist
Jan 28, 2022
Merged

[query] Fix old hl.plot.histogram log argument#11268
danking merged 2 commits into
hail-is:mainfrom
johnc1231:johnc-fix-old-plot-hist

Conversation

@johnc1231

Copy link
Copy Markdown
Contributor

Struct fields are not mutable, so the log argument of hl.plot.histogram would never work. Also have to handle the case where there are 0s

Comment on lines +422 to +427
changes = {
"bin_freq": [math.log10(x) if x > 0.0 else x for x in data.bin_freq],
"n_larger": math.log10(data.n_larger) if data.n_larger > 0.0 else data.n_larger,
"n_smaller": math.log10(data.n_smaller) if data.n_smaller > 0.0 else data.n_smaller
}
data = data.annotate(**changes)

@patrick-schultz patrick-schultz Jan 26, 2022

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.

I think this is wrong, because it plots count-0 and count-1 bins the same. I think the right thing to do is log(x) if x > 0.0 else shift, where shift < 0 is arbitrary, but shift=-1 is as good as anything.

@johnc1231 johnc1231 changed the title [query] Fix old hl.plot.histogram log arguement [query] Fix old hl.plot.histogram log argument Jan 26, 2022
@johnc1231

Copy link
Copy Markdown
Contributor Author

@patrick-schultz , shift = -1 can lead to some pretty ugly graphs when the range of log transformed y values is small (i.e. between 0 and 1)
Screen Shot 2022-01-26 at 2 12 03 PM

@johnc1231

Copy link
Copy Markdown
Contributor Author

The negative 1 doesn't seem right, but we should probably give a warning or something when a user does this. I could log a warning telling user how many points were 0 and so weren't plotted?

@patrick-schultz

Copy link
Copy Markdown
Member

That seems good. Perhaps we could also suggest using hl.plot.pdf instead. Just about to test that out.

@patrick-schultz

Copy link
Copy Markdown
Member

hl.plot.pdf still plots histogram style, not line-plot style, so it still has the issue of where the bottom of the bars go. It would be easy to add a line-plot option, but lets just add the warning for now.

…am would never work. Also have to handle the case where there are 0s
@johnc1231 johnc1231 force-pushed the johnc-fix-old-plot-hist branch from 7683536 to a016948 Compare January 26, 2022 21:11
@johnc1231 johnc1231 added the WIP label Jan 27, 2022
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