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

Fixed doWrap method for vectors #1602

Merged
merged 6 commits into from Oct 16, 2015
Merged

Fixed doWrap method for vectors #1602

merged 6 commits into from Oct 16, 2015

Conversation

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Oct 9, 2015

No description provided.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 9, 2015

Fixes #653 #1560

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 9, 2015

@doutriaux1 @sankhesh please review.

@doutriaux1 how can I create a test that will force the code to duplicate the data?

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 9, 2015

@williams13 if this works, this should fix all of the remaining issues with vector plots and projection. Please test it when you get a chance

@williams13
Copy link
Contributor

@williams13 williams13 commented Oct 9, 2015

Will do! Thanks!

From: Aashish Chaudhary <notifications@github.commailto:notifications@github.com>
Reply-To: UV-CDAT/uvcdat <reply@reply.github.commailto:reply@reply.github.com>
Date: Friday, October 9, 2015 at 12:04 PM
To: UV-CDAT/uvcdat <uvcdat@noreply.github.commailto:uvcdat@noreply.github.com>
Cc: Dean Williams <williams13@llnl.govmailto:williams13@llnl.gov>
Subject: Re: [uvcdat] Fixed doWrap method for vectors (#1602)

@williams13https://github.com/williams13 if this works, this should fix all of the remaining issues with vector plots and projection. Please test it when you get a chance


Reply to this email directly or view it on GitHubhttps://github.com//pull/1602#issuecomment-146962474.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 10, 2015

@doutriaux1 all of the tests are passing.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 11, 2015

@aashish24 please add a test for it. How do you even know it works if you didn't test it?
to test read data let's say between 0 360 and plot -180/180 or vice versa.
to read data within a range:

import cdms2
f=cdms2.open(file)
s=f(data,longitude=(0,360))

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 11, 2015

@aashish24 also your commit quality wasn't what you used us to. No description of what you changed and why. A bit disappointed here 😉

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 12, 2015

@aashish24 I just add a test that shows it is still broken. Run the file with either lon1 = 0. or lon1 = -180. it fails in two different ways.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 13, 2015

@aashish24 also your commit quality wasn't what you used us to. No description of what you changed and why. A bit disappointed here

:) The changes I made should be obvious and since the other lines were not commented, I didn't comment new lines. I am planning to update these classes in the future.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 13, 2015

@doutriaux1 ah.. thanks for the test. that was my other question.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 14, 2015

@doutriaux1 your original test code is buggy, you are getting clt for both u and v. If I modified your code to get the actual vectors, I am getting the right plot. Here is the code:

import cdms2
import vcs
import os
import sys

f=cdms2.open(os.path.join(vcs.sample_data,"clt.nc"))

# -180 failks as well
lon1 = -180
u=f("u")
v=f("v")

u=u(longitude=(lon1,lon1+360.))
v=v(longitude=(lon1,lon1+360.))

x=vcs.init()
Vec = x.createvector()
p = x.createprojection()
p.type = "mercator"
Vec.projection = p
x.plot(u,v,Vec)
raw_input("press enter")

mercator

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 14, 2015

Now I used the mercator. With Polar, I think vector plot will be really bad in general but I think that's a separate issue which I believe I mentioned earlier that vector plot in polar coordinates may require some more work.

@aashish24 aashish24 force-pushed the fix_vector_do_wrap branch from 197a623 to cb57da0 Oct 14, 2015
@aashish24 aashish24 force-pushed the fix_vector_do_wrap branch from d214689 to 60add48 Oct 14, 2015
@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 14, 2015

@doutriaux1 fixed the scaling issue that was causing vectors not to show up in polar coordinates.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 14, 2015

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 14, 2015

@doutriaux1 @sankhesh please approve

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 14, 2015

@aashish24 my original code was NOT buggy, "u" and "v" are wrong in this file and do not get wrapped around by cdms2. Which is why I used "clt" twice, we don't care about how realstic the test is, only that is reproduces the features we are trying to fix.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 14, 2015

@aashish24 you need to at least flake8 it. I will review your test because I'm worried your change to "u" and "v" actually prevents it from reproducing the bug we are trying to fix.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 15, 2015

@doutriaux1 just using CLT twice would not produce any reasonable vectors. I don't understand why we cannot use u and v? for all other tests aren't we using u and v? I was able to reproduce the bug if I don't fix the scale factor. In case of POLAR coordinates the scales were off.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 15, 2015

@doutriaux1 I will fix the style issue. I didn't see them on garant and that's why I thought all is good.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 15, 2015

@aashish24 my original code was NOT buggy, "u" and "v" are wrong in this file and do not get wrapped around by cdms2. Which is why I used "clt" twice, we don't care about how realstic the test is, only that is reproduces the features we are trying to fix.

@doutriaux1 thanks. Is it possible to use better data? If we use clt twice then when projected, vectors will be drawn really bad or not drawn at all.

Removed unused import
@aashish24 aashish24 force-pushed the fix_vector_do_wrap branch from 57798b8 to 60da528 Oct 15, 2015
@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 15, 2015

@doutriaux1 flake8 should be fixed now (had unused import). Also, I tried with clt (for both u and v) works for me. Once you look at it, I can update the test to use clt

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 15, 2015

Okay, tested with clt, vectors look awful but the code seems to be working. @doutriaux1 please let me know if the code is still broken. I saw another issue (not with vectors but with continental lines).

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 15, 2015

@doutriaux1 should be all good. Please review.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 15, 2015

@doutriaux1 ping!

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 16, 2015

@aashish24 aashish24 closed this Oct 16, 2015
@aashish24 aashish24 reopened this Oct 16, 2015
@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 16, 2015

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 16, 2015

@aashish24 catching up... slowly...

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 16, 2015

@aashish24 still fails for ploar is that expected you say?

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 16, 2015

polar

doutriaux1 added a commit that referenced this issue Oct 16, 2015
Fixed doWrap method for vectors

ok let's revisit the polar in another issue
@doutriaux1 doutriaux1 merged commit b2ecd60 into master Oct 16, 2015
6 of 8 checks passed
@doutriaux1 doutriaux1 deleted the fix_vector_do_wrap branch Oct 16, 2015
@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 16, 2015

@doutriaux1 if you use just clt and polar, yes since it will spread them weirdly. If i just use u and v (no wrapping) it does look reasonable but please have a closer look at polar one. All other projections looks great to me.

@aashish24
Copy link
Contributor Author

@aashish24 aashish24 commented Oct 16, 2015

@doutriaux1 thanks 👍 😄 :

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants