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

Add ability to plot waterfalls of delay spectra #158

Merged
merged 12 commits into from
Jul 20, 2018
Merged

Conversation

philbull
Copy link
Collaborator

Plot delay spectra ordered by time or baseline-pair. Solves #155.

@ghost ghost assigned philbull Jul 16, 2018
@ghost ghost added the in progress label Jul 16, 2018
@coveralls
Copy link

coveralls commented Jul 18, 2018

Coverage Status

Coverage increased (+0.005%) to 96.169% when pulling 81c40e3 on delay_waterfall into ac062da on master.

@nkern
Copy link
Member

nkern commented Jul 19, 2018

Thanks for getting a start on this, its much-needed! One comment is, I'm not sure if its right to plot a single waterfall containing multiple baseline-pairs. It seems a little odd to do that, and perhaps is just confusing. Seems like when we say "waterfall", we should mean a single baseline-pair with a non-zero time axis which makes it a heatmap. If we want to make multiple blpair heatmaps, instead of concatenating them into a single heatmap, we should be making multiple subplots... I can make this edit if you are ok w/ it and push to the branch

@philbull
Copy link
Collaborator Author

The idea was that this could eventually be used to make wedge plots too, which is why I allowed it to take multiple blpairs as an argument. It's not quite there yet though (e.g. it doesn't do the necessary averaging for you), so I agree more work is needed.

@nkern
Copy link
Member

nkern commented Jul 19, 2018

Okay, I pushed some edits that, if you aren't a fan of, we can revert. Here is the before & after

For a uvp with three baseline pairs and 3 time integrations per blp, before is
delay_waterfall_v1

after is
delay_waterfall_v2

For me, the importance is the distinction between different blpairs having their own subplot, otherwise it gets hard for me to interpret exactly where one ends and the other begins.

The other important changes are:

  1. the units say log_10 P(k), but the colorbar reads 10^12, 10^13, etc which I think is confusing: if the units are log_10 P(k) the the tick labels should be 12, 13, etc and if its just P(k) then the units should be 10^12, 10^13, etc.

  2. y-axis labeling, which is a little tricky b/c we want to label them in LST which wraps, so you can't just do ax.matshow(..., extent=[dly.min(), dly.max(), lst.min(), lst.max()], but I added some code to do the gymnastics for you

@ghost ghost assigned nkern Jul 19, 2018
Copy link
Member

@nkern nkern left a comment

Choose a reason for hiding this comment

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

This stuff looks good to me. Phil let me know if you have any comments on the edits. I rebased the branch of master already, so should be good to merge once you've had a look.

Also about your comment on wedge plots: I think wedge plots are fundamentally different though. We can't make a "wedge-waterfall" because you've already used up your two heatmap axes for k_perp and k_parallel. A "waterfall" to me implies a time and spectral axes, so if we want to plot wedges, we should probably just make a new function (delay_wedge) to do that.

@acliu
Copy link
Contributor

acliu commented Jul 19, 2018

Is there an option to allow all the subplots to be on a common colour scale?

@nkern
Copy link
Member

nkern commented Jul 19, 2018

@acliu yes there is. you can specify the vmin and vmax parameters and all the colorscales are set to those limits

@nkern
Copy link
Member

nkern commented Jul 19, 2018

@philbull the other important change is that I enforced matplotlib>=2.2 in the setup.py b/c I had matplotlib=2.0 and it failed due to the cbar.get_yticks call (even though I see you had an explicit catch for it, it didn't work for me)... as well as numpy>=1.14 b/c that was causing some problems for me locally due to a numpy==1.13 installation...

@nkern nkern merged commit 28cd602 into master Jul 20, 2018
@ghost ghost removed the in progress label Jul 20, 2018
@nkern nkern deleted the delay_waterfall branch July 20, 2018 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants