Added dxgrow, dygrow to facilitate variable spaced grid. Modified rec… #746
Conversation
…tgrid to generate grid from center outward using growth factor
daveh150
requested review from
apcraig,
eclare108213,
dabail10,
phil-blain and
JFLemieux73
There was a problem hiding this comment.
Several thoughts.
I think we also want a flag to turn this on. Setting dxgrow and dygrow = "1.0" and it actually being 1.0_real8 is a little risky. I know 1.0 is likely to turn into 1.0_real8, but I still don't like to think about a regular grid spacing case being roundoff non-regular.
I'm not sure I love setting all work_g1 values to the "center" and then adjusting outwards. I also wonder if there is a way to write a cleaner single loop that doesn't require two identical loops (center out x 2) to initialize things. The loops also impose things on the loop ordering (i/j) that seems complicated.
I also think it would be good to more tightly tie the lon/lat and the dx/dy calculations. For instance, rather than independently computing lon/lat then dx/dy from the namelist parameters, why not compute lon/lat then set dx=lon(i+1)-lon(i), etc. Or compute dx and dy then compute lon/lat from dx and dy. That way we're sure the grid values are self-consistent. With constant spacing, it's not as risky.
Maybe there should be a simple computation of dxrect and dyrect first then lon/lat should be derived from those? Maybe consistency and loops get cleaner?
The definition of dxgrow, dygrow ("growth factor") should be spelled out a little more clearly somewhere. The documentation also needs to be updated, at least to add the new namelist variables to the appropriate sections in the user guide.
|
Thanks @apcraig! I was trying to avoid extra flags and if statements, but good point about roundoff issues. I can add a flag to the namelist to enable grid expansion/telescoping and only do math if the expansion is desired. For variable dx/dy it probably make sense to define the dx spacing first, then convert from centimeters to radians for the lat/lon arrays. Maybe rearranging that operation in the code will help. If it is preferred to grow from the bottom left corner, or if growing out from the center really doesn't matter, I can just apply the multiplication from the index (1,1) and not worry about expanding from the center. I'll work on all this next. |
|
Thanks @daveh150. I don't have a strong feeling about whether to start in the corner or center. I assumed there was a reason center was chosen, namely to change the gradient over the domain. If you start in a corner, you will probably end up with increasing/decreasing dx/dy over the entire domain. In the center, you have decreasing to increasing (or vv). Think about it a bit, not sure what the best setup is. |
|
I had assumed that starting from the center made the edges more similar in size for possibly testing periodic or open boundary conditions.
|
|
@eclare108213, good point. |
|
Simplifying the loops and/or logic would be good, and I agree that multiplying by a factor that might change the results in standard cases should be avoided, if possible. I think it's better to start the calculation from the center so that there's not a big jump in grid size in cyclic configurations (in case someone tries such a test). But if that makes the logic/loops more complicated, then start in the corner and we can add a warning not to run rectgrid tests with cyclic boundaries and variable grid spacing. Just a hint re the template above: removing the space from between the brackets fixes the check-box formatting, e.g.
Alternatively, you can submit the PR with out editing the check-boxes, then click on the ones you want to check off, afterwards in the open PR. |
…le_dxdy flag to check of want grid spacing scaled. Renamed dxgrow/dygrow to dxscale/dyscale.
…spacing scale factors
|
Thanks for the feedback and reviews! I added flag scale_dxdy to apply the grid expansion scaling. I also renamed dxgrow/dygrow to dxscale,dyscale. I reworked the looping so it only loops over half the nx_global/ny_global - 1, then multiplies the scaling factor accordingly. I know it is still a relative complicated loop, but with multiplying by the cell next to it I'm not sure if there is a more elegant way. Any other thoughts are appreciated. I tried to explain what is happening in the comments. I added a method to check for even/odd in nx_global/ny_global. That affects the center of the domain. But I'm not sure if we would ever use an odd number for a domain size in these tests? If not I can remove that to avoid confusion. I did a cursory update to the documentation to add the new scaling variables. and a vargrid_test suite, but that needs to be updated to add more cases (I think). |
There was a problem hiding this comment.
A couple comments.
I propose we not include vargrid_suite.ts. It looks like this was for quick testing and debugging, but probably should not be included formally. Those tests already exist in gridsys_suite.
I'm wondering about the hardcoded lonref and latref. Should these be namelists?
|
I agree making the lonref/latref namelist seems to make sense. They probably should be renamed something like grid_lonref/grid_latref to signify it is for grid generation and not diagnostics or something else. Also fine to remove the vargrid_suite.ts. I wasn't sure if we wanted the variable spaced test it's own suite or lumped in with the gridsys_suite.ts. I can add a vargrid option to all the the boxsym tests for starters. |
|
@daveh150, I think it's fine to add a few more vargrid tests to the existing test suites, but we probably don't need more than a handful of tests total to provide reasonable test coverage. We just want to test the feature, not necessarily all uses. I also think we should get rid of vargrid_suite.ts. That kind of suite is what I often create for development, as needed. But we don't need to check all those in. Lets also add lon/lat rect grid reference point to the namelist. The default should be whatever is hardwired into the code right now (Barrow?). I would call the variables lonrefrect, latrefrect which is sort of consistent with dxrect and dyrect. But open to other ideas. Thanks! |
|
I just ran a full gridsys_test and have 3 test fails. remarkably, they are not the ones with a variable grid test, but they are gridcd tests. Do these normally fail? [hebert@boulder:daveh150/testsuite.lonlatref]$ more fails.ts Test Grid PEs Setssmoke gbox80 2x2 boxwallblock,gridcd |
…te.ts. Updated the box nml to specify the default grid_lonref and grid_latref for future reference.
|
@apcraig All sounds good. I removed the vargrid_suite.ts and made grid_lonref, grid_latref namelist variables. I just pushed that for review. This had those three gridcd failures I don't understand. Depending on the test they are failing with (abort_ice) error = (ice_write_hist)ERROR: writing variable vvelE |
|
Thanks @daveh150 for the most recent changes. Could we change grid_lonref and grid_latref to lonrefrect and latrefrect. I think it's important to have "rect" in the name. Also, I don't think we need so many vargrid tests. I would have 2 or 3 for each grid type (B, C, CD). Please pick a subset of the tests you've added and remove the rest. Some of the gridcd tests do fail on some machines. I have not looked closely at that right now. It may also depend what values uninitialized arrays are taking on. I actually think we need to reduce our gridcd testing at this point. I can also run the gridsys_suite on cheyenne once your branch is ready to make sure nothing new is broken. Did you run a baseline suite prior to your changes to check that you're not changing answers and to check that no new tests are failing? You might want to create a sandbox with the unmodified code and create baseline results, then use --bcmp to make sure answers are the same and nothing new is failing. |
…lt vargrid tests to 3 per B,C,CD grid.
|
Done, Renamed grid_lonref/grid_latref to lonrefrect/latrefrect. Also kept only the 3 directional tests without islands in the gridsys_suite.ts. The others are commented out. |
|
P.S. Sorry I forgot to change my calendar and missed the meeting today. :-( |
|
Thanks @daveh150, this looks good. Just for sanity, I'll run a test suite on cheyenne with these changes. If all OK, we can merge today. |
|
The test suite indicates that a number of test fail regression. Looks like answers changed for all these cases, I think a goal was to make sure the answers did not change. Looking at diff from output, the first changes are in the lon/lat diagnostics, My guess is that this is an issue of setting in the namelist. We should probably either remove this and leave it as default (set to double precision in the code) or change it to double precision in the namelist. Let me test this theory and I'll propose some further modifications. |
|
Fixing the double precision of the lon/lat refrect did not fix the problem. I still think we need to change to double precision in the ice_in by adding d0 to lon/lat refrect. We should also remove setting lon/lat refrect in the set_nml options where it is the same as the base ice_in, which I think is everywhere. I think the problem comes down to the numerics of computing the lons/lats in subroutine rectgrid. The old implementation was numerically different in terms of computing dx,dy,lon,lat. At this point, I would have two sections of code. If scale_dxdy is false, do it the old way. If scale_dxdy is true, do it the new way. One could also argue that we should have a requirement that scale_dxdy=true and dxscale=dyscale=1.0d0 should reproduce bit-for-bit the old implementation. And maybe that is a reasonable requirement. The new implementation would probably not meet that requirement. Anyway, I think we need to do some refactoring after we decide what the requirements are. I like the requirements I propose above, old implementation is bit-for-bit. New var grid is bit-for-bit with old implementation if dxscale=dyscale=1.0d0. I'm open to discussing that second requirement, but we could add a test for it in our test suite quite easily. Happy to discuss these issues. @daveh150, please do regression testing as you move forward if you can. I'm happy to do another set of tests on cheyenne once things are updated too. |
|
Thanks Tony. I can add d0 to the ice_in and remove them from the set_nml. It seems we have a convention of listing EVERY namelist option in ice_in, so adding d0 to those seems to be clearest. I'll rework the recgrid to keep what was there originally and do something different if scale_dxdy = true. I'll also see about setting up regression test on our local machines here. |
…move explicit latrefrec/lonrefrect from set_nml. Make dxscale,dyscale,latrefrec,lonrefrec double precition in ice_in
|
Modified ice_in to be double precision for the scale factors. I added a separate subroutine rectgrid_scale_dxdy to apply the scaling if scale_dxdy=true. This is called from within rectgrid if scale_dxdy = true. If scale_dxdy=false it will run the original rectgrid method. If I did this correctly, I ran a regression by first checking out the main branch then running gridsys_suite.ts with --bgen cice.main option. I then checked out the box_vres branch and ran the same gridys_suite.ts and compared with the --bcmp cice.main. results are as follows: Both branches have the same 6 failures due to the gridcd tests more fail.ts Test Grid PEs Setssmoke gbox80 2x2 boxwallblock,gridcd The 18 missing are the vargrid tests that do not exists in the main branch. I also added a set_nml.scale1 to apply dxscale,dyscale = 1.d0. But I'm not sure how to apply a regression test if this is applied, since it will be a separate directory. I probably just don't know now to tell the test suite which directories to compare. Is the --bdir is for a testsuite output? Thank you! |
|
There are conflicts in this branch. A couple PRs were merged recently, one did a bunch of cleanup. There is a short section in one file that needs to be addressed. I suggest a rebase. Or you can do it on the "Resolve conflicts" button above. If you need help, let me know. Once that's resolved, I'll retest on cheyenne. thanks! |
|
I merged in the updates from main into the box_vres branch. Thanks. |
|
I ran a full test suite on cheyenne with 3 compilers and everything looks good. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#d0d777e03ded31a19e3aea531095a90b0d6697e0 |
|
Thank you Tony! I added lonrefrec and latrefrect to the documentation. Looks like the other namelist additions are there too. |
|
@daveh150, one more thing. The namelist documentation is alphabetical in the tables, could you update the order of the new namelist in ug_case_settings.rst. Otherwise, documentation looks fine. Sorry I didn't notice that before. I think we can then merge. Thanks! |
|
Thank you, I did not notice they were alphabetical order. I put them in alphabetical order and pushed the update. Thanks! |
CICE-Consortium#746) * Added dxgrow, dygrow to facilitate variable spaced grid. Modified rectgrid to generate grid from center outward using growth factor * Adding vargrid namelist options. * Refactored rectgrid to compute dx/dy first. Then ULON/ULAT. Added scale_dxdy flag to check of want grid spacing scaled. Renamed dxgrow/dygrow to dxscale/dyscale. * Added method to check for odd nx_global/ny_global when applying grid spacing scale factors * Update comments before computing dx/dy * Update comments when checking for even/odd * made grid_lonref, grid_latref namelist varaibles. Removed vargrid_suite.ts. Updated the box nml to specify the default grid_lonref and grid_latref for future reference. * Change grid_lonref/grid_latref to lonrefrect,latrefrect. Reduce default vargrid tests to 3 per B,C,CD grid. * Make new subroutine rectgrid_scale_dxdy to implemnet grid scaling. Remove explicit latrefrec/lonrefrect from set_nml. Make dxscale,dyscale,latrefrec,lonrefrec double precition in ice_in * Add set_nml.scale1 to test vargrid with dxscale/dyscale = 1.d0 * Remove lonrefrec/latrefrec from boxnodyn * Add lonrefrect, latrafrect to documentation * Inserted new scaled grid varibles in alphabetical order in documentation
…tgrid to generate grid from center outward using growth factor
For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers
PR checklist
Add growth factors in x and y direction to facilitate variable spaced grid
David Hebert
ENTER INFORMATION HERE
Added x and y growth factors to test variable spaced grid. The intent is to grow the grid spacing from center of grid outward. I added a few tests to the gridsys_suite that passed, but can use help with deciding what other tests to setup.
Here is what the HTN and HTE look like for the boxsyme, kmtslands test:
HTE
HTN