adding estNsq.py by hajsong · Pull Request #2 · altMITgcm/verification

hajsong · 2017-05-24T20:33:52Z

A python script to estimate N^2 using T and S

edoddridge

Looks good @hajsong - I've commented on the commit with some suggestions/questions.

edoddridge · 2017-05-24T20:41:27Z

so_box_biogeo/diags/estNsq.py

@@ -0,0 +1,75 @@
+import numpy as np
+from MITgcmutils import rdmds, densjmd95
+from hspython import loadgrid


Would it be better to include the function loadgrid locally, rather than require users to go out and obtain the code for it separately?

Yes, that makes sense. But it is likely that I will use this function quite a lot in the future.
I will put a file with "loadgrid" in this directory for now.

edoddridge · 2017-05-24T20:42:34Z

so_box_biogeo/diags/estNsq.py

+Nsq = - dRHOdr*g/rhoconst*grd.mskC
+#
+# Now, estimate Nsq using T and S. 
+# When computing "drhodr" at the layer interface, density at upper and lower cell 


Rather than "layer interface" it may be clearer to say the interface between tracer cells.

Sure. I will change that

edoddridge · 2017-05-24T20:43:36Z

so_box_biogeo/diags/estNsq.py

+showx = 5
+scale = 1e5
+
+X, Y = np.meshgrid(grd.YC[:, 0], grd.RC[:showz])


Why are these called X and Y rather than Y and Z?

No special reason.. Y and Z make more sense and I will change that.

edoddridge · 2017-05-24T20:46:38Z

so_box_biogeo/diags/estNsq.py

+
+im = ax[2].contourf(X, Y, Nsq_ra[:showz,:,showx]*scale, np.arange(0,15.1,1), cmap='Reds')
+cb = plt.clabel(im,colors='black',fmt='%3.1f')
+ax[2].set_title('N$^2$, from RHOAnoma [x 10$^5$ s$^{-1}$]', color='black', fontsize=15)


Some axes labels would make these plots clearer.

Sure, I will add labels.

edoddridge · 2017-05-24T20:48:24Z

so_box_biogeo/diags/estNsq.py

+"""
+    Estimating Brunt-Vaisala frequency using T and S
+    In case when N$^2$ is needed but do not have 'DRHODR' saved, 
+    one can still estimate it using T and S


Is it worth adding "and the appropriate equation of state" to the end of this docstring?

Yes. I will add that.

“estNsq.py” is updated and a file “mitgcmgrid.py” is added.

edoddridge · 2017-05-25T14:05:17Z

The code looks good to me.

@jahn, @jm-c, @christophernhill - any of you want to chime in?

A couple of comments on using git for the future:

commits should generally represent single logical changes.
- For example, adding the mitgcmgrid.py file and changing the import statement for loadgrid could be considered a single logical change that should probably be introduced in a single commit without any other changes.
- This makes it easier for future people to see what has been done, why it was done, and if it causes issues in the future, it's much easier to undo if each change has its own commit.
commit messages should generally detail exactly why this change was required and why it was the best way to accomplish that outcome.
- For example ""estNsq update" doesn't give much insight into why the changes in this commit are a good idea.
- This commit makes changes so that the script can be run after testreport has generated the outputs, right? That's something that future collaborators will probably want to know.
These blog posts have some helpful advice about commits and commit messages.

hajsong · 2017-05-25T14:20:50Z

Ed's comments make sense to me.
I will keep those guidelines in mind next time I request a pull.

adding estNsq.py

b2cbb1d

A python script to estimate N^2 using T and S

edoddridge reviewed May 24, 2017

View reviewed changes

estNsq update

53748da

“estNsq.py” is updated and a file “mitgcmgrid.py” is added.

Conversation

hajsong commented May 24, 2017

Uh oh!

edoddridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoddridge commented May 25, 2017

Uh oh!

hajsong commented May 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants