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

panel_13 #131

Merged
merged 20 commits into from
Jul 27, 2020
Merged

panel_13 #131

merged 20 commits into from
Jul 27, 2020

Conversation

hCraker
Copy link
Contributor

@hCraker hCraker commented Jul 1, 2020

Closes #130
image

Plots/Panels/NCL_panel_13.py Show resolved Hide resolved
Plots/Panels/NCL_panel_13.py Outdated Show resolved Hide resolved
Plots/Panels/NCL_panel_13.py Show resolved Hide resolved
@hCraker hCraker requested a review from clairefio July 2, 2020 20:10
@hCraker
Copy link
Contributor Author

hCraker commented Jul 6, 2020

image

@hCraker hCraker requested review from clyne and jukent July 10, 2020 15:15
@clyne clyne removed their request for review July 10, 2020 16:33
@hCraker hCraker requested a review from clyne July 13, 2020 19:42
Copy link
Collaborator

@clyne clyne left a comment

Choose a reason for hiding this comment

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

Rose, the arrow glyphs in the Python version are slightly shorter, and thicker than the NCL version. As a result many of the smaller magnitude glyphs - in the equatorial region, for example - degenerate to blobs that don't really have a discernible orientation. Could you try tweaking these a bit and see if you can improve the clarity?

@hCraker
Copy link
Contributor Author

hCraker commented Jul 15, 2020

I tried to make the arrows more legible. Is this good @clyne?
image

@hCraker hCraker requested a review from clyne July 15, 2020 18:39
@clyne
Copy link
Collaborator

clyne commented Jul 16, 2020

That's better, but some of the arrows are still pretty much blobs unless you zoom in a lot. I see what the problem is; if you set the scale factor to something like 200 the smallest arrows are visible, but then the others are too long. I believe NCL has a notion of minimum arrow size, and I think @michaelavs or @clairefio may have run into this. I'm not sure if there is a simple solution, but perhaps they have some thoughts. If there is not a simple workaround, how about adding a comment that some arrows aren't legible, and opening an issue on geocat-viz to provide a means to set a minimum arrow size?

@hCraker
Copy link
Contributor Author

hCraker commented Jul 16, 2020

@clyne I have found a simple way to set minimum lengths using some trig. It works fine, but I'm afraid it is misleading because I had to manipulate the data to produce the desired lengths. Is this best practice? I could add a comment to the documentation explaining how this may not be a desirable feature when trying to make truly accurate plots.
image

@jukent
Copy link
Collaborator

jukent commented Jul 17, 2020

@clyne I have found a simple way to set minimum lengths using some trig. It works fine, but I'm afraid it is misleading because I had to manipulate the data to produce the desired lengths. Is this best practice? I could add a comment to the documentation explaining how this may not be a desirable feature when trying to make truly accurate plots.

What type of data manipulation are you referring to?

@hCraker
Copy link
Contributor Author

hCraker commented Jul 17, 2020

@clyne I have found a simple way to set minimum lengths using some trig. It works fine, but I'm afraid it is misleading because I had to manipulate the data to produce the desired lengths. Is this best practice? I could add a comment to the documentation explaining how this may not be a desirable feature when trying to make truly accurate plots.

What type of data manipulation are you referring to?

I'm searching through the data for any wind vector with a magnitude less than minLength and then changing those vectors' components to make their magnitude equal to minLength

@jukent
Copy link
Collaborator

jukent commented Jul 17, 2020

I'm searching through the data for any wind vector with a magnitude less than minLength and then changing those vectors' components to make their magnitude equal to minLength

It might be more honest to not display wind vectors below that minimum length than to change their size, but an argument could be made for still displaying wind direction. I would feel better about this if it were somehow indicated in a key (size X represents wind of value X and below, for example). Thoughts @clyne ?

@clyne
Copy link
Collaborator

clyne commented Jul 21, 2020

I'm searching through the data for any wind vector with a magnitude less than minLength and then changing those vectors' components to make their magnitude equal to minLength

It might be more honest to not display wind vectors below that minimum length than to change their size, but an argument could be made for still displaying wind direction. I would feel better about this if it were somehow indicated in a key (size X represents wind of value X and below, for example). Thoughts @clyne ?

I don't like the idea of misrepresenting the arrow length without clearly indicating in the plot that not all arrows are drawn to scale. Ideally, a different glyph would be drawn to indicate this. I don't think we're going to solve this problem with the time remaining and suggest falling back to the illegible glyphs and a comment in the documentation for the plot.

@hCraker
Copy link
Contributor Author

hCraker commented Jul 21, 2020

@clyne @jukent I have opened an issue regarding this discussion so we remember to fix this later. Please let me know if the added comment in the latest commit needs clarification.

@clyne
Copy link
Collaborator

clyne commented Jul 21, 2020

Thanks, Rose.

@clyne
Copy link
Collaborator

clyne commented Jul 22, 2020

Thanks, Rose.

Rose, I don't see a comment in the example about the glyph legibility issue.

@hCraker
Copy link
Contributor Author

hCraker commented Jul 22, 2020

Rose, I don't see a comment in the example about the glyph legibility issue.

I was having issues with git, but I just pushed the change. Sorry about that @clyne

Copy link
Collaborator

@clyne clyne left a comment

Choose a reason for hiding this comment

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

Terrific!

@clyne clyne requested a review from erogluorhan July 25, 2020 14:26
Copy link
Collaborator

@erogluorhan erogluorhan left a comment

Choose a reason for hiding this comment

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

Nice work!

@erogluorhan erogluorhan merged commit 5c165d9 into NCAR:master Jul 27, 2020
@hCraker hCraker deleted the panel_13 branch July 28, 2020 20:07
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.

panel_13
6 participants