I'm excited for this PR because I'd like to use the same infrastructure to do time-variable ice-shelf pressure soon.

Here's an example of activating a package:
https://github.com/sbrus89/MPAS-Model/blob/ocean/time-varying-wind/src/core_ocean/driver/mpas_ocn_core_interface.F#L246-L259

@xylar Thanks for the input! I'll work on creating packages as we discussed.

Try the delinter on .F and .xml files that you edited, here:
MPAS-Tools/source_code_processing/mpas_source_linter/mpas_source_linter.py
mpas_source_linter.py -f foo.F
and it makes a foo.e file.

@xylar Take a look if you get a chance and let me know if you have any other suggestions.

It looks like you'll eventually need to rebase onto ocean/develop and handle whatever conflicts in driver/mpas_ocn_core_interface.F may emerge. Shouldn't be too bad (I hope).

I found my notes for new code additions:
Doug wrote a nice tool to help us write uniform code. It is here:
https://github.com/MPAS-Dev/MPAS-Tools/tree/master/source_code_processing/mpas_source_linter
and can be run with:

linter -f file.F

it produces a new file that notifies you of 'delint' clean-up items, that sometimes cause a problem on odd compilers. These can also be helpful for brand new code:

sed -i 's/    /\t/g' $file    # change space to tab, for *.xml files
sed -i 's/[ \t]*$//g' $file   # remove trailing white spaces

though it is best to clean up just the new code you are submitting, and not include white space alterations to lots of other code in this PR.

@sbrus89, thanks for getting this PR in before leaving for the break. I'm going to wait to review until you do the rebase and address edits from @xylar and @mark-petersen (thank you to you both for the speedy review!).

I can also take a closer look, test, and review on the next round, after the holiday. @sbrus89 thanks for pushing this through. I return to work January 7.

@xylar I just did the rebase to handle the conflicts in driver/mpas_ocn_core_interface.F.

@mark-petersen I did my best to address the whitespace issues with the linter and sed commands in 27fea306.

I think I addressed all of @pwolfram 's suggestions in these last few commits. Let me know if there's any other issues that need to be taken care of. I appreciate all the feedback!

@sbrus89, at this point, I think this branch has become a mix of at least 2 projects. I've been there and I know how that happens. Maybe it would be worthwhile at some point to break out commits related to specific test cases from those related to the time-varying atmospheric forcing itself. It would, further, be useful to separate SOMA commits from your new hurricane test case to the degree feasible. Perhaps I can work with you on this when I'm there in just over a week?

@xylar, thanks for all the help and engaging here!

I have a question / comment that has me a bit confused. I'm thinking it is more worthwhile to have this committed sooner than later since this is holding up a variety of other work (e.g., your ocean/ice shelf coupling, wetting-drying development [general ALE, split explicit], the start of some hurricane simulations). I agree we probably don't need SOMA and a hurricane to show viability of time varying forcing. But, I do think the test case with SOMA should be enough and self contained to verify this is working at least reasonably. We don't, very intentionally, need to consider the processor decomposition bug here.

My primary concern is that this has dragged on for a while, for a variety of viable reasons, but it seems like we may be putting on the breaks and slowing down the process for an artificial reason at this point that may impede progress in other areas, e.g., just for "cleaner" PRs. I'm all for that (perhaps to a fault as we have worked through in the past) but am wondering in this context if the other competing goals of this being a key input to our other work supersedes the "cleanness". In this context, I would argue we can merge even if there has been some scope creep here (but potentially the hurricane simulation should be removed from this specific PR). Steven and I would like to complete this and move on to complete other goals. My original plan was to help out a bit early next week on this to get this merged so we can use this in all our other work. Please let me know what you think and what else I'm missing that informs this discussion-- we really appreciate your engagement here and support of this work.

@pwolfram, I really do think it's important to clean this up. 58 commits is too many to feasibly go back and debug if issues come up and a lot of them should be squashed.

I also feel pretty strongly that test cases and code changes should be kept in separate pull requests. This is not just for cleanliness but also to avoid confusion -- neither the SOMA test case nor the hurricane test case are directly related to the time-varying forcing, they're just used to test it.

My suggestion to you and @sbrus89 would be to create another branch for each test case you've created or updated here. Then, you should rebase this branch onto ocean/develop, keeping only commits related to time-varying forcing (squashing where you can). Then, you should rebase each of your other branches onto this branch, removing commits already in this branch (and again squashing where appropriate). It's a tedious process but there should be a minimum of conflicts unless commits have somehow involved both code changes and test case changes (which is exactly what I'm hoping to carefully avoid by keeping branches and PRs separate) and I think the whole thing should only take an hour or 2.

Again, I'm happy to help sit down with @sbrus89 when I'm there next week. If you get to this before then, great! But I'm not really okay with just merging a branch with this many seemingly unrelated commits, even in the interest of efficiency.

Thanks @xylar. Will you be in on Monday? If so, we can certainly wait.

My biggest concern above is that I think we do want test cases to accompany commits (probably not quite the case here, so your argument holds minimally for the hurricane case). Otherwise, we can't be sure new code is working correctly. Ideally, we have a unit test that verifies correctness of the new code that is added.

This being said, if we want separate PRs for code / test cases, I would argue they need to be sequential and immediately merged next to each other on the same day so there isn't a merge in between them that could modify behavior of the testing case of the new code, but perhaps this isn't as hard a line as I would think.

We definitely want to get @mark-petersen's perspective on this repo management strategy we are discussing here.

@pwolfram, if a version of a test case is specifically being designed to test the functionality being added, or if a test case is being modified specifically to make use of the new functionality, I can see why both should be part of the same PR. To the extent possible, I would still suggest rebasing before the PR gets merged to try to have a set of commits related to code changes followed by a set of commits to add the test case (or visa versa).

If a new test case is being added that will have a life beyond this functionality, as is my sense with the hurricane test case, I think it would be more appropriate to keep that project separate. It should be possible to have a branch base off of this branch where that new test case is being updated as this branch changes. If it's appropriate to merge that other branch right after this one, that's great! But I don't quite see why it's critical to have both merged together unless there's a regression test or unit test as part of that work that tests this PR's functionality.

Anyway, I know we have pretty different styles of development and I wanted to put in a strong pitch for the Jacobsen/@xylar style of having PRs with fewer commits and smaller scope where possible.

@mark-petersen, I complied with intel debugging flags and ran the SOMA time varying forcing case. It ran without any errors. I also squashed in a very minor change to the python script that cases the forcing file for the SOMA case (the pressure needed to be made absolute instead of gage). That's why there's an extra force push.

Thanks @sbrus89. There is a problem with my last E3SM/MPAS PR. I need to look at that before I merge this one.

No problem @mark-petersen, I appreciate all your help with this!

@mark-petersen, I just did a rebase onto ocean/develop.

Merged this version locally, tested in E3SM with a variety of tests. Passes:

PASS ERS_Ld3.T62_oQU240.GMPAS-IAF.cori-knl_intel RUN time=228
PASS PEM_Ln9.T62_oQU240.GMPAS-IAF.cori-knl_intel RUN time=202
PASS PEM_Ln9.ne30_oECv3wLI.A_WCYCL1850-DIB-ISMF_CMIP6.cori-knl_gnu RUN time=621
PASS SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.cori-knl_gnu RUN time=715
PASS SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.cori-knl_intel RUN time=3080

Congratulations, @sbrus89. This was a long process!

Agreed, excellent work @sbrus89 and thanks for the help and coordination @mark-petersen and @xylar!