MITgcmutil update #727

dramauh · 2023-04-17T14:18:19Z

What changes does this PR introduce?

I have updated the python package MITgcmutils including new modules

What is the current behaviour?

The current version lacks of basic python functions to open and manipulate binary output from MITgcm

What is the new behaviour

(if this is a feature change)?
The updated library has converted MATLAB functions such gen_blanklist, readbin and writebin. It also includes all the density profiles in MITgcm, and a function to convert depth to dbar following MITgcm. Finally, there are some new functions in utils.py for tile visualization and/or debugging.

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)
The documentation needs a slight update to include the new functions and the changes in the old ones

Other information:

I wrote a symbolic version of 0.5. I let the developers decide the most suitable version number

Suggested addition to `tag-index`

(To avoid unnecessary merge conflicts, please don't update tag-index. One of the admins will do that when merging your pull request.)

... and linebreaks for lines longer than 79 characters

mjlosch · 2023-04-18T09:40:19Z

When I include MITgcmutils.utils in doc/utilities/utilities.rst I get error messages like:

/Users/mlosch/MITgcm/review/utils/python/MITgcmutils/MITgcmutils/utils.py:docstring of MITgcmutils.utils.tilecmap:30: CRITICAL: Unexpected section title.

Test
----

This is from the doc in the python function and seems to be triggered by the preceding

    Example
    -------
    >>> [fig]=tilemap(bathy, 51, 51,)
    >>> [fig]=tilemap(bathy, 51, 51,58,5)

When I move Test before Example the errors go away. I have no idea how to fix this.

mjlosch · 2023-04-18T09:43:16Z

My previous (first) comment got scrapped, so I repeat what I can remember of it: @dramauh Thanks for this contribution. I took the liberty to make small changes to make the doc-work. Not sure if I did it correctly ( @jahn ?). Please check if it works for you and please add missing documentation. When I tried, I ran into the problem described above.

dramauh · 2023-04-18T11:50:35Z

@mjlosch is fine, I should have tried to compile the document before commiting. That error catched me by surprised Thank for correcting the problem

small bugged in density.py

dramauh · 2023-04-19T06:01:50Z

I fixed a small flag in the density.py. I also corrected documentation adding utils and tests. I did not get any error @mjlosch, and I wrote exactly the same code that you use..very strange

mjlosch · 2023-04-19T07:00:58Z

Thanks, I can now also compile the readthedocs without error messages. It may have to do with the infamous DOS eol characters (^M) that I saw in a few places (e.g., the utils.py file is a DOS file, something you may want to change).

mjlosch · 2023-04-19T08:10:50Z

@dramauh thanks, the docs look good. I'll have a look at the actual python code with the help of @jahn

dramauh · 2023-04-19T09:19:04Z

fantastic. I hope everything is clear. Let me know if there is anything that needs further developement.

mjlosch · 2023-04-19T11:09:49Z

@dramauh I am sorry, I was wrong. The problem described above remains. The docstring sections in utils.gen_blanklist and utils.timecmap starting with "Test" lead to the above error message. I don't know what this is.

- add __all__ to tests/__init__.py - remove second code block, as it does not seem to be allowed

mjlosch · 2023-04-19T15:31:47Z

The docs were not rendering correctly for both MacOS and Linux (my MacOS implementation gave a warning, see above, the Linux version didn't). Apparently, only one block of code is allowed.

mjlosch

This looks good. I didn't test everything thoroughly, but I did find a small problem in diagnostics.readstats, which I took the liberty to fix. @dramauh please check, if it still works for you. I also changed back the files modes to 644 (rw-r--r--) for all regular files (except the script gluemncbic).

The tests with the large (1MB) bathy_test.bin only work, if one is in the tests directory. Maybe that can be made more foolproof (e.g. by specifying a path to the file?). I am not sure. Maybe @jahn has an opinion about this.

The automodule documentation seems to work now.

mjlosch · 2023-04-27T13:24:02Z

utils/python/MITgcmutils/CHANGES.txt

@@ -1,3 +1,33 @@
+Version 0.5, 2023-04-17


I have no feelings about this., but the next step would be 0.2, wouldn't it?

mjlosch · 2023-04-27T13:28:28Z

utils/python/MITgcmutils/MITgcmutils/conversion.py

+gravity  = -9.810000e+00
+rhoConst =  1.027500e+03
+eosRefP0 =  1.013250e+05


These parameters are a matter of choice and as such they should be defaults, but not constants.
One can also imagine making gravity a function of latitude (see UNESCO Tech. Pap. in Mar. Sci., 1983, eq 27):

grav = 9.780318 * (1.0 + (5.2788e-3 + 2.36e-5 * sin^2(lat)) * sin^2(lat))

I agree with you. We could add those constant as default input parameters in the function. Also I have been thinking to remove that minus and move it to the equation..It does not make much sense there. I try to correct it tomorrow.

I have included suggestions in conversion and test utils. Also, I fixed a small bug in density

dramauh · 2023-04-28T14:29:31Z

I added @mjlosch suggestions. Now, you can add latitude to the function pfromz. I also added paths to locate the binaries that are used in the tests.

I added mds to init because it is not called by default in linux. I cleaned conversion module to accept different dimensions for lat

mjlosch · 2023-05-04T12:14:21Z

@jahn when you find the time, it would be great if you have a quick look at this. I think this is good to merge.

I have deleted the bathy_test.bin and added bathy data to test_utils.py.

dramauh · 2023-08-03T07:51:51Z

I removed the bathy file and included a portion in the test_utils.py to save some space. I wonder if the code looks untidy because of this. @mjlosch may you take a look, please？ I can always change the file back and add a separate data file (~20kb).

mjlosch · 2023-08-04T09:19:33Z

I am fine with that, no more large bathymetry file! I would probably for the plots of timecmap use default figure size and fonts. On my small screen I cannot see the entire figure without resizing.

dramauh · 2023-08-20T03:08:02Z

I deleted the figure and font size properties and fixed a few things

edoddridge · 2024-02-05T00:11:47Z

I'm taking a look at this at the moment. I'll have a review by the end of the week. Updates soon.

edoddridge

These are great additions to the package. I've left a few comments about things to improve or change. I haven't yet had a chance to actually use the new functions on some data, so the comments are, for now, focused on testing frameworks and coding style.

I'll try and make time to point the functions at some data and actually try them out before ocean sciences, but I might not manage it.

utils/python/MITgcmutils/MITgcmutils/conversion.py

edoddridge · 2024-02-09T00:49:45Z

doc/utilities/utilities.rst

+This Python package includes a number of helpful functions and scripts for
+dealing with MITgcm output.  You can install it from the model repository (in
+directory :filelink:`utils/python/MITgcmutils`) or from the Python Package
+Index:


The line jitter in this file is a bit annoying. It's not really a problem, but the file history and pull request would both be cleaner if removed.

I think this was done by another author. I decided to leave it as it is part of the readocs

edoddridge · 2024-02-09T01:04:43Z

utils/python/MITgcmutils/MITgcmutils/density.py

+
+    Notes
+    -----
+    AUTHOR:     Martin Losch 2002-08-09  (mlosch@mit.edu)


Really? I'm skeptical that @mjlosch wrote this routine in 2002. It looks like this comment has been copy/pasted into the new doc strings.

I guess at that time, I wrote a Matlab version of this (did I even know that python existed?), see utils/matlab/dens*.m, which was later converted to python.

As Martin said, some of these routines came from Matlab scripts and others from the MITgcm source code. I tried to keep the author comments.

👍
I think that's a great approach in general.

I queried this one because I wasn't aware that the TEOS-10 routines existed in any format in 2002. But, it's before my time, so I don't actually know.

hhahah, I have just noticed it..I will correct those time travel typos

edoddridge · 2024-02-09T01:05:31Z

utils/python/MITgcmutils/MITgcmutils/density.py

+    """
+    Computes in-situ density of sea water
+
+    Density of Sea Water using TEOS 48-polynomial.


Change to "TEOS-10"

edoddridge · 2024-02-09T01:09:46Z

utils/python/MITgcmutils/MITgcmutils/jmd95.py

@@ -1,222 +0,0 @@
-#
-# created by mlosch on 2002-08-09
-# converted to python by jahn on 2010-04-29


Only half of this provenance has been kept in the new doc string.

edoddridge · 2024-02-09T01:29:31Z

utils/python/MITgcmutils/MITgcmutils/tests/test_utils.py

+bathy=np.array(bathy).astype('f4').reshape([60,60])
+
+def test_blanklist():
+    """Test blanklist generator


As written, I don't think it's possible for these tests to fail under python's pytest framework.

We need an assert statement or a comparison with blessed output that can fail in order for this to be an automated test.

Once that exists, we can add some code to the automated testing (.github/workflows/build_testing.yml) to run them every time.

edoddridge · 2024-02-09T01:30:56Z

utils/python/MITgcmutils/MITgcmutils/utils.py

+
+
+def hfac(depth,rF,hFacMin=0.3,hFacMinDr=50,htype='C'):
+    """


I can't find a test for this function.

I add a new example for this function

edoddridge · 2024-02-09T01:33:45Z

utils/python/MITgcmutils/MITgcmutils/utils.py

+    return tuple(hfac_dic[i] for i in htype)
+
+def readbin(fname, ndims, dataprec='float32', machineformat='b'):
+    """


We could have a circular test for the read and write binary functions. Make an array, write it to binary, read it back in, and then assert that the original array is identical to the one that has been through both functions.

I did not add it to prevent making dummy files. I prefer to wait for more complex functions before using circular tests.

do not modify salinity (to be debated)

mjlosch · 2024-05-07T20:33:50Z

@jahn and I went through the file density.py and simplified a few things. The check for dimensions relies on python's ability to broadcast the shapes of different fields and avoids the Matlab-methods inherited from the seawater library. @dramauh Please check if you can live with that.

I also have a suggestion for the "tests": as they are not unit or regression tests, but rather a useful illustration of gen_blanklist and tilecmap, I suggest to change the corresponding names to something like "examples".

dramauh · 2024-05-08T08:38:01Z

I have just taken a a look to it. I think it is much cleaner now. Thanks for your help. About the tests, I have not preference. I can change the name of those functions. The aim of that folder was to collect all test and/or examples as the library becomes bigger

mjlosch · 2024-05-08T12:49:30Z

I think renaming the directory "tests" to "examples" would be good and maybe even rename test_utils.py. @dramauh please go ahead an do so.

I am not a good resource for giving advice on how to design automated tests. I just learned something about that yesterday (thanks to @jahn).

mjlosch · 2024-05-08T12:58:12Z

utils/python/MITgcmutils/MITgcmutils/utils.py

+
+    fig = plt.figure()
+    ax = fig.add_subplot(111)
+    ax.pcolor(mland,cmap=cmap_lm)


here and elsewhere: is there any reason to prefer pcolor over the (in my experience) much faster pcolormesh?
I tried pcolormesh and the figures look the same.

I changed this as you suggested

jahn · 2024-05-09T15:35:23Z

I did some more cleanup. utils.py still needs a bit of attention.

Another issue is that before this PR, one could import MITgcmutils without having matplotlib installed, as long as one didn't explicitly import cs or llc. Now everything is imported automatically and plotting and non-plotting capabilities are more mixed up.

The test folder was renamed examples and their functions to eg_... There is a new function in examples to compute the vertical mask grid. Lastly, I edited a few comments and replace pcolor to pcolormesh

MITgcmutil update

2f1200b

jm-c requested review from jahn and mjlosch April 17, 2023 15:28

stuff to make the automodule work for density and llc

a05cbbb

... and linebreaks for lines longer than 79 characters

density and doc

ff649ce

small bugged in density.py

converted dos to unix format

aa464aa

fix some rendering and readthedocs compilation errors

954a455

- add __all__ to tests/__init__.py - remove second code block, as it does not seem to be allowed

mjlosch added 3 commits April 27, 2023 15:22

small fix to allow multiple lines starting with "# Field"

42f2646

change file modes back to 644

c0b9b25

change more file modes back to 644

70428a4

mjlosch approved these changes Apr 27, 2023

View reviewed changes

small things and inclusion of spatially varying gravity

d6e3479

I have included suggestions in conversion and test utils. Also, I fixed a small bug in density

dramauh and others added 3 commits April 28, 2023 22:44

fix rendering docs

05d3a9b

mds init and some cleaning

4fbebfe

I added mds to init because it is not called by default in linux. I cleaned conversion module to accept different dimensions for lat

small tweaks

1f773fa

clearing some space

9cc91f8

I have deleted the bathy_test.bin and added bathy data to test_utils.py.

plot adjustments

123388d

edoddridge self-requested a review February 5, 2024 00:11

edoddridge requested changes Feb 9, 2024

View reviewed changes

Fix ini_poly3, simplify dimension check and salinity check

0c1a75f

do not modify salinity (to be debated)

Merge upstream/master into mitgcmutils_update to fix docs-problem

19307da

mjlosch reviewed May 8, 2024

View reviewed changes

mjlosch and others added 3 commits May 8, 2024 09:06

remove some left over commented and extra blank lines

b2191bf

Fix some doc strings

5640bfa

Clean up and fix conversion module

c1efdc0

jahn and others added 6 commits May 9, 2024 15:20

Fix some docstrings

651696a

Clarify docstring

04b2ccd

Few changes in the folders

0330655

The test folder was renamed examples and their functions to eg_... There is a new function in examples to compute the vertical mask grid. Lastly, I edited a few comments and replace pcolor to pcolormesh

small typo

d31a261

correcting doc

6786fb0

edit rad conversion

2f8eaac

dramauh requested a review from edoddridge May 11, 2024 13:39

minor changes to doc-strings

0eb835e

MITgcmutil update #727

Are you sure you want to change the base?

MITgcmutil update #727

Conversation

dramauh commented Apr 17, 2023 • edited

What changes does this PR introduce?

What is the current behaviour?

What is the new behaviour

Does this PR introduce a breaking change?

Other information:

Suggested addition to tag-index

mjlosch commented Apr 18, 2023

mjlosch commented Apr 18, 2023

dramauh commented Apr 18, 2023

dramauh commented Apr 19, 2023

mjlosch commented Apr 19, 2023

mjlosch commented Apr 19, 2023

dramauh commented Apr 19, 2023

mjlosch commented Apr 19, 2023

mjlosch commented Apr 19, 2023

mjlosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dramauh commented Apr 28, 2023 • edited

mjlosch commented May 4, 2023

dramauh commented Aug 3, 2023

mjlosch commented Aug 4, 2023

dramauh commented Aug 20, 2023

edoddridge commented Feb 5, 2024

edoddridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjlosch commented May 7, 2024

dramauh commented May 8, 2024

mjlosch commented May 8, 2024

mjlosch May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahn commented May 9, 2024

dramauh commented Apr 17, 2023 •

edited

Suggested addition to `tag-index`

dramauh commented Apr 28, 2023 •

edited

mjlosch May 8, 2024 •

edited