Skip to content

Conversation

@vanroekel
Copy link
Contributor

This will update the 30to60 test case to allow for use of the Met Office EN4 1900 state estimate as the ocean initial condition

@vanroekel vanroekel requested a review from xylar August 27, 2020 04:38
@vanroekel vanroekel marked this pull request as draft August 27, 2020 04:39
@vanroekel
Copy link
Contributor Author

This PR depends on #668 and #669

@vanroekel
Copy link
Contributor Author

@xylar and @mark-petersen I was trying to test this one a bit since it has a new initial condition, but the test is getting stuck in the cull mesh step. I ran for two hours and it did not finish the cull step. Have you seen this before? I'm running on grizzly with compass 0.1.8.

@vanroekel
Copy link
Contributor Author

the 1900 state estimate has been uploaded to the anvil public_html directory.

@mark-petersen
Copy link
Contributor

The cell culler step is very slow. We are planning to work on that. It shouldn't take 2 hours for an EC60to30, though. If this one only changes the bathymetry, the culled mesh is the same as the r03. I also just made one earlier today that made it through the cell culler:

/lustre/scratch3/turquoise/mpeterse/runs/200826_test_compass/ocean/global_ocean/EC60to30/init/culled_mesh

so you could link to those files for the initial_state step where bathymetry is added.

@vanroekel vanroekel force-pushed the ocean/updateEC6030_add_EN4_1900_IC branch from b227df5 to a07246d Compare August 27, 2020 16:49
@vanroekel vanroekel marked this pull request as ready for review September 3, 2020 21:48
@xylar
Copy link
Collaborator

xylar commented Sep 4, 2020

@vanroekel, do you want to change the driver so it build both the usual initial condition and the 1900 initial condition? It seems like that could be a reasonable default. Do we want a separate spin-up for the 1900 as well?

@xylar xylar force-pushed the ocean/updateEC6030_add_EN4_1900_IC branch 2 times, most recently from 9ce5f50 to 7d731f0 Compare September 4, 2020 16:19
@xylar
Copy link
Collaborator

xylar commented Sep 4, 2020

I'm seeing a seg fault in initital_state_1900 on Cori. The error message isn't helpful and I'll have to rerun in debug mode to see if I can figure it out. But it seems like this one isn't ready to merge yet.

@vanroekel
Copy link
Contributor Author

@xylar I agree, I'm going to move this back to draft to address the issues. I'll also rework the driver to spinup the 1900 estimate too.

@vanroekel vanroekel marked this pull request as draft September 4, 2020 17:30
@xylar
Copy link
Collaborator

xylar commented Sep 4, 2020

It worked fine when I rebuilt in debug mode on Cori. I'm going to try again because my earlier test was with a test merge rather than the rebased version that is here now. Maybe I made a mistake or something.

@xylar
Copy link
Collaborator

xylar commented Sep 4, 2020

Seems to be a false alarm or something that somehow got fixed by the rebase. I'm able to run the initial_state_1900 just fine now.

@xylar
Copy link
Collaborator

xylar commented Sep 4, 2020

@vanroekel, I'll hold off on reviewing this until you have it out of draft mode, but everything is working as expected for me. I ran initial_state_1900 and then manually linked spin_up to point to that initial condition instead of initial_state. Then, I ran e3sm_coupling pointing to the results of that spin-up. e3sm_coupling is still running but no signs of trouble.

I think we need to decide what our needs are here. EC30to60kmL60E3SMv2r04 isn't really a different mesh or bathymetry than EC30to60kmL60E3SMv2r03, it's just a different initial condition on the same mesh. Presumably, we have other ways of handling that already in E3SM, right? So maybe we need to treat this PR as producing a second initial condition for EC30to60kmL60E3SMv2r03 rather than a new mesh? I think we probably want the COMPASS workflow to reflect that.

@mark-petersen mark-petersen force-pushed the ocean/updateEC6030_add_EN4_1900_IC branch from 7d731f0 to 49b3a5e Compare September 5, 2020 03:59
@mark-petersen mark-petersen force-pushed the ocean/updateEC6030_add_EN4_1900_IC branch from 774db6b to 4f85037 Compare September 5, 2020 04:11
@mark-petersen
Copy link
Contributor

I might have fixed the problem. On the new 1900 directories, topography file variable names were x,y,z in the namelist, but were supposed to be lon,lat, bathymetry.

@mark-petersen
Copy link
Contributor

I think a new initial condition deserves a new revision number for clarity. It is also how we name the initial files. So I would vote for r04.

@vanroekel
Copy link
Contributor Author

@mark-petersen and @xylar I was wondering if it was possible to alter the test case such that in setup we could add a flag to setup_testcases to set which condition we use? I don't want to lose the ability to use PHC, it would be nice if we could choose to spinup the PHC temp/salinity or the EN4 by some flag, but don't know if that's possible. It may be easier and more straightforward to configure the driver to run both inits and spin ups. What do you think?

@xylar
Copy link
Collaborator

xylar commented Sep 5, 2020

@vanroekel, I don't think it makes sense to alter the way setup_testcase.py works for this purpose. That would be a pretty significant overhaul, would require changes to devleop rather than just ocean/develop and doesn't fit with the v2 timeline.

Instead, I think it makes sense to reorganize init and spin_up for EC60to30 into:

build_mesh/base_mesh
build_mesh/culled_mesh
spin_up_PHC/initial_state
spin_up_PHC/prep_spin_up1
spin_up_PHC/simulation
spin_up_PHC/files_for_e3sm
spin_up_1900/initial_state
spin_up_1900/prep_spin_up1
spin_up_1900/simulation
spin_up_1900/files_for_e3sm

Would it be okay if I take a first stab at this, basing off this branch?

@xylar
Copy link
Collaborator

xylar commented Sep 5, 2020

@vanroekel, I gave it a try. Please take a look at my branch https://github.com/xylar/MPAS-Model/tree/ocean/updateEC6030_add_EN4_1900_IC

In particular, I did the following:

  • added spin_up_EN4_1900 and renamed spin_up to spin_up_PHC
  • moved initial_state steps (60 and 64 layer) to these spin_up cases
  • moved e3sm_coupling to the spin_up cases and renamed it files_for_e3sm to hopefully make @jonbob happier (I will eventually do more major overhaul on all the cases). To accommodate the rename, I modified the script for generating E3SM files to take a config file with a different name.
  • Modified driver and other configs to accommodate the reorg.
  • Switched back to EC30to60kmL60E3SMv2r03 as the mesh name because these changes don't really require a new mesh name
  • Added the initial condition to the description of the mesh (but it could be made more verbose, e.g. a citation or link)

I'll test this out to make sure I didn't make mistakes (who am I kidding, to find out what mistakes I inevitably made). I'll report back.

@xylar
Copy link
Collaborator

xylar commented Sep 5, 2020

After a few small fixes, my branch seems to be working for me. Results are on Grizzly in:

/lustre/scratch4/turquoise/xylar/test_EC30to60kmL60E3SMv2r03_EN4_1900/ocean/global_ocean/EC60to30/spin_up_*

@mark-petersen
Copy link
Contributor

@xylar, I like the idea of putting the initial_state step within the spin-up. That step is fast and multi-core, while the base_mesh and cull_mesh steps are slow and single core, so it's an easier way to organize for running as well.

With your proposal r03 can now mean two different initial condition data files - is your proposal to carry around the _EN4_1900 suffix? The two different bathymetries are also in the initial_state step. I guess if we end up with a larger number of initial_state options, we just have the suffix match the directory name.

@xylar
Copy link
Collaborator

xylar commented Sep 8, 2020

With your proposal r03 can now mean two different initial condition data files - is your proposal to carry around the _EN4_1900 suffix?

It seems problematic to me if different initial conditions for the same mesh get different names. This would mean new mapping files get generated both to bring this mesh into E3SM and to perform diagnostics later.

Surely, there is a way to distinguish an initial condition from a mesh in E3SM. For example, suppose I want an appropriate initial condition for an 1850 run vs. a 1950 or 2020 run. My proposal would be for the 1900 initial condition to be appropriate for some E3SM compsets and the PHC initial condition to be appropriate for other compsets but for both to have the same mesh name.

@vanroekel
Copy link
Contributor Author

Surely, there is a way to distinguish an initial condition from a mesh in E3SM. For example, suppose I want an appropriate initial condition for an 1850 run vs. a 1950 or 2020 run. My proposal would be for the 1900 initial condition to be appropriate for some E3SM compsets and the PHC initial condition to be appropriate for other compsets but for both to have the same mesh name.

This is definitely possible we have a fair number of initial conditions for the oEC60to30v3 mesh as one example (G-case, B-case, Spun-up B-case) that all use the same mesh and reside in the same folder

@milenaveneziani
Copy link
Contributor

I agree, I don't see why we would want a specific mesh to be named after an initial condition. It's the other way around: initial condition file named after the mesh.

@xylar
Copy link
Collaborator

xylar commented Sep 8, 2020

The two different bathymetries are also in the initial_state step.

@mark-petersen, my understanding was that we are using r02 and r03 to decide if WC is fine with just moving to GEBCO/BedMachine, in which case we would no longer need to maintain ETOPO1 as an option for any future EC30to60km meshes. That is certainly my hope.

@mark-petersen
Copy link
Contributor

Yes, I see the logic now of having the mesh revision name really be specific to the horizontal mesh (steps base_mesh and cull_mesh), and not the bathymetry or initial T&S (step initial_state). That also has the advantage that we track new E3SM coupling files with the revision number (horizontal mesh) and can have various files for values of T&S during the spin-up process. Thanks for the helpful discussion.

@xylar
Copy link
Collaborator

xylar commented Sep 9, 2020

I disagree regarding the bathymetry. A different bathymetry is a different mesh, just as a different number of vertical levels is a different mesh.

@xylar
Copy link
Collaborator

xylar commented Sep 9, 2020

The discussion made me realize that we want only one file_for_e3sm stage per global_ocean resolution. In my example branch, I have a separate one for each initial condition, but that would lead to confusion since both would need to be updated each time there's a new mesh version. I'll add a new test and case for that part so it's clearer.

@xylar
Copy link
Collaborator

xylar commented Nov 6, 2020

@vanroekel, this PR is going to move over to the new COMPASS repo: MPAS-Dev/compass#13

I'll ping you when it's ready for your attention. I went ahead and did it "my way" instead of your way, just in terms of the organizaiton of the tests and steps. This is the same as what I had done in #740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants