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

Pass the log state to the histogram machinery #1963

Merged
merged 2 commits into from
Mar 29, 2019

Conversation

cphyc
Copy link
Contributor

@cphyc cphyc commented Mar 22, 2019

This fixes #1962.

@cphyc cphyc changed the title This fixes #1962. Pass the log state to the histogram machinery Mar 22, 2019
@cphyc
Copy link
Contributor Author

cphyc commented Mar 22, 2019

Using the same dataset as #1962 I know have the following plot
working_example

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just a small comment below, and otherwise can you add an entry under 0.14.3 in CHANGES.md?

xmin = np.log10(xmin)
xmax = np.log10(xmax)
xmin = np.log10(xmin) if xmin > 0 else np.log10(xmax) - 10
xmax = np.log10(xmax) if xmax > 0 else np.log10(xmin) + 10
Copy link
Member

Choose a reason for hiding this comment

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

This might give strange results if both xmin and xmax are zero for some reason - I think it might be best to actually just return an array of NaN values if either xmin or xmax are negative rather than modify the limits. You can construct this array efficiently with:

broadcast_to(np.nan, bins)

could you update this here and below?

@codecov
Copy link

codecov bot commented Mar 23, 2019

Codecov Report

Merging #1963 into master will increase coverage by 0.05%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1963      +/-   ##
==========================================
+ Coverage   87.48%   87.54%   +0.05%     
==========================================
  Files         244      244              
  Lines       21716    21775      +59     
==========================================
+ Hits        18999    19062      +63     
+ Misses       2717     2713       -4
Impacted Files Coverage Δ
glue/viewers/scatter/state.py 91.82% <ø> (ø) ⬆️
glue/core/data.py 90.22% <50%> (ø) ⬆️
glue/viewers/matplotlib/layer_artist.py 95% <0%> (-2.5%) ⬇️
glue/viewers/histogram/layer_artist.py 93.05% <0%> (-0.7%) ⬇️
glue/_mpl_backend.py 100% <0%> (ø) ⬆️
glue/_deps.py 66.25% <0%> (+2.45%) ⬆️
glue/viewers/matplotlib/viewer.py 97.82% <0%> (+2.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ab4614...4d17185. Read the comment docs.

@astrofrog
Copy link
Member

I'll go ahead and merge this, thanks @cphyc!

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.

Density plots in scatter 2d not using the log parameter
2 participants