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 plot azimuthal cross-section to radardisplay class. #405

Merged
merged 2 commits into from Oct 23, 2015
Merged

Add plot azimuthal cross-section to radardisplay class. #405

merged 2 commits into from Oct 23, 2015

Conversation

nguy
Copy link
Contributor

@nguy nguy commented Oct 21, 2015

This PR is in response to Issue #398

@@ -1041,6 +1180,35 @@ def _get_ray_data(self, field, ray, mask_tuple, filter_transitions):

return data

def _get_azimuth_rhi_data_x_y_z(self, field, target_azimuth,
edges,mask_tuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a style issue:
This line is not properly aligned with "self" in the line above and there should be a space after the comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@jjhelmus
Copy link
Contributor

This looks really great @nguy, thanks for adding this. A few comments, ideas:

  • It looks like your editor is not stripping trailing whitespace. Removing trailing whitespace is a good practice and recommended by PEP8, Python's style guide. A google search should pull up instructions on how to configure your editor to do this automatically.
  • Any change you could add a unit test for the new method? test_xsect.py and test_radar_display.py should give you some examples. For graphing there is typically no artifacts to tests, the assumption is that if the code run and a plot was generated then everything is good. Not exactly the best testing mentality but better than nothing.
  • The gatefilter and filter_transitions parameters are not used in the method. I think they can be passed to the _get_azimuth_rhi_data_x_y_z method and used to filter the data in the same manner as is used by the _get_data method.

Again thanks for the code!


if mask_tuple is not None:
mask_field, mask_value = mask_tuple
mdata = self.fields[mask_field]['data'][ray]
Copy link
Contributor

Choose a reason for hiding this comment

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

ray is not defined in this scope and I don't believe a slice is required here at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, good catch. The dangers of copy and paste!

@nguy
Copy link
Contributor Author

nguy commented Oct 23, 2015

@jjhelmus Thanks for the tip, I wondered why I always seemed to have these issue!
I just adjusted my settings, so hopefully this will help. What editor do you use? It seems like you told me vim?

I had gatefilter and filter_transitions in there, not sure why I took those out.

I'll take a look at the test files you mention and see what I can do.

Fix typos.
Add test unit.
@nguy
Copy link
Contributor Author

nguy commented Oct 23, 2015

Let me know if that test works for you. Other comments are added.

@jjhelmus
Copy link
Contributor

I use vim/Gvim/Macvim depending on the computer within reach and my mood.

I have the following in my .vimrc for removing whitespace. It does it automatically when .py files are saved and I can fit F5 to do manually.

" Functions
function! <SID>StripTrailingWhitespaces()
    " Preparation: save last search, and cursor position.
    let _s=@/
    let l = line(".")
    let c = col(".")
    " Do the business:
    %s/\s\+$//e
    " Clean up: restore previous search history, and cursor position
    let @/=_s
    call cursor(l, c)
endfunction

" call with <F5> and turn on for all Python files
nnoremap <silent> <F5> :call <SID>StripTrailingWhitespaces()<CR>
autocmd BufWritePre *.py :call <SID>StripTrailingWhitespaces()

I also put a bunch of other stuff in my .vimrc and .gvimrc files, all of them are in my dotfiles repository

@jjhelmus
Copy link
Contributor

Looks good to me. I'll merge this once the AppVeyor tests pass. Thanks again @nguy.

jjhelmus added a commit that referenced this pull request Oct 23, 2015
Add plot azimuthal cross-section to radardisplay class.
@jjhelmus jjhelmus merged commit df37ead into ARM-DOE:master Oct 23, 2015
@nguy
Copy link
Contributor Author

nguy commented Oct 23, 2015

You bet, I figured I could get this in quickly and give myself a feeling of accomplishment this week. :)

@scollis
Copy link
Member

scollis commented Oct 23, 2015

Nice work @nguy !!

-sent from a mobile device-

On Oct 23, 2015, at 3:45 PM, Nick notifications@github.com wrote:

You bet, I figured I could get this in quickly and give myself a feeling of accomplishment this week. :)


Reply to this email directly or view it on GitHub.

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.

None yet

3 participants