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

[WIP] Log axes 2d #316

Merged
merged 20 commits into from
Feb 26, 2018
Merged

[WIP] Log axes 2d #316

merged 20 commits into from
Feb 26, 2018

Conversation

doutriaux1
Copy link
Contributor

needs: #315

@doutriaux1
Copy link
Contributor Author

@danlipsa @scottwittenburg I would like to merge this shortly in master, can you review please, it supersedes #315
Baselines are at:
CDAT/uvcdat-testdata#188

@doutriaux1 doutriaux1 mentioned this pull request Feb 20, 2018
@scottwittenburg
Copy link
Collaborator

Whew, huge change! I mostly skimmed the parts I wasn't familiar with, but no potential issues popped out at me.

+1

@doutriaux1
Copy link
Contributor Author

@danlipsa actually I did not do the vector or streamline graphic method. I don't quite follow the logic on how to propagate the axes to the vector arrays. I'll double check streamlines, I think I just forgot about them but if they store their own vtk copy of the data then i think it will be up to you again.

@doutriaux1
Copy link
Contributor Author

@danlipsa might be worth merging this in first and then fix vector/streamlines separately in another PR.

axisConvertFunctions = {
"linear": {"forward": lambda x: x, "invert": lambda x: x},
"area_wt": {"forward": lambda x: numpy.sin(x / 180. * numpy.pi),
"invert": lambda x: numpy.arcsin(x) / numpy.pi * 180.},
Copy link
Contributor

Choose a reason for hiding this comment

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

What is area_wt?

@danlipsa
Copy link
Contributor

danlipsa commented Feb 21, 2018

@doutriaux1 Should be the same as the rest of the 2D plots, isn't it?

@danlipsa
Copy link
Contributor

Looks good to me. Nice work.

@danlipsa
Copy link
Contributor

Sure we can merge this and do the vectors and streamlines in a different PR.

@doutriaux1
Copy link
Contributor Author

@danlipsa make sure you review this first though, there's a lot going on here.

@doutriaux1
Copy link
Contributor Author

@danlipsa when I apply to vectors (area_wt on y axis) I get tiny arrows.

@doutriaux1 doutriaux1 merged commit e721529 into master Feb 26, 2018
@doutriaux1 doutriaux1 deleted the log_axes_2d branch February 26, 2018 23:45
@danlipsa
Copy link
Contributor

@doutriaux1 I have a question on axisconvert: What is the purpose of area_wt. What does wt stand for?

@danlipsa
Copy link
Contributor

@doutriaux1 @aashish24 @scottwittenburg You have 'xyscale' for both axes but 'xaxisconvert' and 'yaxisconvert' for individual axes. It would be nice to make them consistent.
What about xyaxisscale, xaxisscale, yaxisscale?

@danlipsa
Copy link
Contributor

We could change only xaxisconvert and yaxisconvert to xscale and yscale

@doutriaux1
Copy link
Contributor Author

@danlipsa area_wt weights (hence the wt) the latitudes by their area, essentually it's using sine(lat).

@doutriaux1
Copy link
Contributor Author

@danlipsa where do you see xscale and yscale?

@danlipsa
Copy link
Contributor

danlipsa commented Feb 28, 2018

I don't. These are the suggested new names for xaxisconvert and yaxisconvert so that they are consistent with xyscale.

@doutriaux1
Copy link
Contributor Author

@danlipsa ok, I guess I'm wondering where do we use xyscale 😄

@danlipsa
Copy link
Contributor

That's a minor issue. Grepping for xscale I can see we use that with a different meaning. Maybe:
xyaxisconvert (current name is xyscale)
xaxisconvert
yaxisconvert

@doutriaux1
Copy link
Contributor Author

@danlipsa I see it now, there's indeed an xyscale function to set both xaxisconvert and yaxisconvert on the graphic method. I didn't even know about it! Feel free to rename the function or simply remove it, I don't it's ever used and it's really just bloatware in my opinion.

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