Hello @sbrus89! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file compass/ocean/tests/spherical_harmonic_transform/qu_convergence/__init__.py:

Line 23:80: E501 line too long (96 > 79 characters)

• In the file compass/ocean/tests/spherical_harmonic_transform/qu_convergence/analysis.py:

Line 30:80: E501 line too long (97 > 79 characters)

• In the file compass/ocean/tests/spherical_harmonic_transform/qu_convergence/init.py:

Line 16:80: E501 line too long (97 > 79 characters)

• In the file compass/ocean/tests/spherical_harmonic_transform/qu_convergence/mesh.py:

Line 30:80: E501 line too long (97 > 79 characters)

Comment last updated at 2022-05-13 18:27:20 UTC

@xylar, this is still a work in progress, but I thought I'd submit the PR to get some initial feedback.

@sbrus89, it turns out flake8 is already part of the compass development environment:

I will add it to MPAS-Analysis, though.

@sbrus89, is flake8 not pointing out lines longer than 79 characters and indentation that's not a multiple of 4? If not by default, maybe there's a setting for flagging those, too?

@xylar, it definitely is pointing those out. I just took a break because I respectfully disagreed with it on the remaining issues.

I'm afraid I do insist on tabbing being a multiple of 4 and I will accept long lines but only under unusual circumstances.

I'm afraid I do insist on tabbing being a multiple of 4 and I will accept long lines but only under unusual circumstances.

That's my fault, I thought it was complaining about the function lines I broke up. I agree about the 4 spaces.

BTW, I know this PEP8 stuff is super annoying. I didn't appreciate it when Phil Wolfram insisted on it in my early python days.

The way I explain it at this point is that it's the difference between a native python "speaker" and someone who "speak"s with a light (or a strong) Fortran accent. We can understand each other but it takes longer and it would make things smoother if we all stuck to the standard way of speaking, particularly when all of our code can easily become the example that the next developer works from.

Its no problem, having a uniform standard is worth my being slightly annoyed for 10 minutes. Mostly I misunderstood what it was flagging.

@sbrus89, please rebase and remove whatever commit(s) updated the E3SM-Project and MALI-Dev submodules. You can make updates to them locally for your testing but you should not commit changes to these submodules. We update them only in their own PRs and only with a review process that makes sure they don't introduce any unexpected changes.

It looks like the submodule changes were probably committed accidentally in cee46a1, your most recent commit. If you can edit that commit to remove those changes before you rebase, I think it will make your life easier.

@xylar, sorry about the submodules getting mixed up. I've fixed that and rebased.

@sbrus89, testing on Anvil with Intel and Intel-MPI, I'm seeing this for QU60_init_serial_40:

Running: srun -n 36 ./ocean_model -n namelist.ocean -s streams.ocean
Abort(538976288) on node 6 (rank 6 in comm 0): application called MPI_Abort(MPI_COMM_WORLD, 538976288) - process 6
srun: Job step aborted: Waiting up to 62 seconds for job step to finish.
slurmstepd: error: *** STEP 546864.16 ON b733 CANCELLED AT 2022-04-15T10:43:48 ***
srun: error: b733: tasks 0-5,7-35: Killed
srun: error: b733: task 6: Exited with exit code 1

Could this be out of memory?

@xylar, did you happen to compile with USE_SHTNS=true with E3SM-Project/E3SM#4472?

Oops, nope. I used that branch but not the build flag. Sorry!

No, not at all; that's my fault. you wouldn't have known unless I told you!

I'm trying again. Now, when I build with make USE_SHTNS=true OPENMP=true intel-mpi on Anvil, I'm seeing:

src/libdycore.a(sht_init.o): In function `init_sht_array_func':
sht_init.c:(.text+0x2d2cd): undefined reference to `ffly_m0'
sht_init.c:(.text+0x2d2d5): undefined reference to `ffly_m0'
sht_init.c:(.text+0x2d2dd): undefined reference to `ffly_m0'
sht_init.c:(.text+0x2d2e5): undefined reference to `ffly_m0'
sht_init.c:(.text+0x2d30d): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d315): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d31d): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d325): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d379): undefined reference to `ffly'
sht_init.c:(.text+0x2d381): undefined reference to `ffly'
sht_init.c:(.text+0x2d389): undefined reference to `ffly'
sht_init.c:(.text+0x2d391): undefined reference to `ffly'
sht_init.c:(.text+0x2d3b9): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d3c1): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d3c9): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d3d1): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d3f9): undefined reference to `fomp_a'
sht_init.c:(.text+0x2d401): undefined reference to `fomp_a'
sht_init.c:(.text+0x2d409): undefined reference to `fomp_a'
sht_init.c:(.text+0x2d411): undefined reference to `fomp_a'
sht_init.c:(.text+0x2d439): undefined reference to `fomp_b'
sht_init.c:(.text+0x2d441): undefined reference to `fomp_b'
sht_init.c:(.text+0x2d449): undefined reference to `fomp_b'
sht_init.c:(.text+0x2d451): undefined reference to `fomp_b'
sht_init.c:(.text+0x2d479): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d481): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d489): undefined reference to `ffly_m'
sht_init.c:(.text+0x2d491): undefined reference to `ffly_m'

I rebuild without OPENMP=true and it worked fine. I didn't run a long enough job to test everything but the 60km tests seemed to work. I will see if I just messed up the OPENMP=true compile and also run the complete tests when I can.

One thing to consider is whether it makes sense to add another, much cheaper test that could be used for regression testing.

@sbrus89, in addition to #383, this PR still needs documentation. Is that something we talked about already earlier today? I think so but similar meetings are getting muddled in my brain...

@xylar, I added the user's and developer's guide documentation. I built it and it seemed to work for me. Let me know if you'd like to see any changes. Thanks!

Great, @sbrus89! I'm doing a test merge with master and running some tests now.

thanks for your help and patience, @xylar!