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

Minor fixes for elm-pb and python tools #1958

Merged
merged 3 commits into from
Apr 7, 2020
Merged

Minor fixes for elm-pb and python tools #1958

merged 3 commits into from
Apr 7, 2020

Conversation

dschwoerer
Copy link
Contributor

Resolves #1071

* Remove dependence on Bunch
  only partially tested. elm-pb plotting works now.

* fix permissions on nc file

* remove code with hardcoded filename, also no shebang was set

* add missing she-bang to elm-pb script
@ZedThree
Copy link
Member

Thanks! Is there supposed to be a boututils/bunch.py in this PR?

To be honest, I think there's a ton of stuff in pylib we can just delete in v5.

Copy link
Contributor

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a couple of comments where it would be nice to be more consistent with normal Python conventions for array order, if anyone feels like taking the time to update them.

@@ -30,8 +29,6 @@ def moment_xyzt( sig_xyzt, *args):#rms=None, dc=None, ac=None):
#; -AC (DC subtracted out), i.e., a function of (x,y,z,t)
#;-------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't changed in this PR, but since we're touching the methods here...
This moment_xyzt seems to be mis-named - it's actually using (t,x,y,z) (I guess this is due to a port from IDL to Python). It would be nice to change the names and comments so that they reflect the actual (t,x,y,z) behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I would rather remove them.
I think what the function does is to simply to put actually in a library ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have strong feelings about it. Given what @ZedThree said in the BUG meeting today about getting rid of stuff in the old pylib, I'd vote for leaving as is for now (get this PR merged), and remove stuff from pylib in a dedicated PR.

if s == 4 :
nx = np.shape(var)[1]
ny = np.shape(var)[2]
nt = np.shape(var)[0]

result = np.zeros((nx,nt))
for t in range (nt):

result[:,t] = surface_average(var[t,:,:,:], g, area=area)
result[:,t] = surface_average(var[t,:,:,:], grid, area=area)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to switch the result so it's result[t,x].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather remove it ... If you think this function is useful I'd do it though ...

bendudson
bendudson previously approved these changes Mar 13, 2020
@dschwoerer
Copy link
Contributor Author

Thanks! Is there supposed to be a boututils/bunch.py in this PR?

Yes. I had git cleaned it, but found it in a backup.

@ZedThree ZedThree merged commit 62b9551 into next Apr 7, 2020
@ZedThree ZedThree deleted the minor-aux branch April 7, 2020 09: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.

None yet

4 participants