Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove/fix close_boundaries and update box2001 and boxslotcyc tests #541

Closed
apcraig opened this issue Dec 11, 2020 · 7 comments · Fixed by #610
Closed

Remove/fix close_boundaries and update box2001 and boxslotcyc tests #541

apcraig opened this issue Dec 11, 2020 · 7 comments · Fixed by #610

Comments

@apcraig
Copy link
Contributor

apcraig commented Dec 11, 2020

The close_boundaries option does not work. This option needs to be fixed or removed. Currently, box2001 and boxslotcyc use close_boundaries and turning that option off changes answers, so it will continue to live in the code until some decisions are taken. See also #540.

@TillRasmussen
Copy link
Contributor

I am currently looking at the "freya_gnu_restart_gbox80_1x1_box2001" as a test case for the 1d solver as it is the only gbox case that is not bit for bit within the 1d and 2d comparison.

Can you elaborate a little on what does not work when the "close_boundaries option is chosen?
All it does is to add 2 rows/columns of land.

@apcraig
Copy link
Contributor Author

apcraig commented May 4, 2021

@eclare108213 should clarify, but I believe closed boundaries do NOT work properly and should never have been set. This is on our list of tests to review and fix. It may involve setting the mask to land on the edges, but I'm not totally clear on that.

@eclare108213
Copy link
Contributor

Unfortunately I don't remember the details, only the agony of trying to fix the problems and failing. I can go in to my office at some point and find my notes. It's possible that imposed land boundaries obviate the issues, so that whether the boundaries are set as open or closed doesn't matter, but if changing from one to the other is not BFB in @apcraig 's tests then there's still a problem. The thing I couldn't get to work was the halo cells in the corners of the grid -- I couldn't make them work consistently. What I'm not sure of is whether the answers would change when the grid was rotated (e.g. replace x with y etc), or if I was running into some other issue.

@TillRasmussen
Copy link
Contributor

TillRasmussen commented May 4, 2021

Ok, I will ignore that it fails then. At least for now.

@apcraig
Copy link
Contributor Author

apcraig commented May 20, 2021

I have been looking at this issue/PR. I think we can close it.

What the box2001 and boxslotcyl do is set open boundaries, but then use the namelist flag close_boundaries=.true. What that does is set a layer of two gridcells of land on the boundary. This should be fine. The thing that doesn't work is setting ew/ns_boundary_type to 'closed'. If a user sets the boundary_type to 'closed', the model now aborts. So I think this issue was created in error by me due to some confusion.

Does anyone disagree? If nobody says anything, I'll close this in the next couple days. In summary

  • ew/ns_boundary_type = 'closed' does not work and aborts in CICE now.
  • close_boundaries = .true. creates a 2 gridcell wide land mask in the grid at initialization.
  • box2001 and boxslotcyl use close_boundaries=.true. to create a land mask on the boundary.
  • turning off close_boundaries in the box2001 and boxslotcyl should change answers, so that is fine.

@eclare108213
Copy link
Contributor

That makes sense, and yes it is confusing. Could you check the docs to see whether this sufficiently explained there? Maybe an update there is all that is needed. (Or maybe we just need to be reminded to look at the documentation...)

@apcraig
Copy link
Contributor Author

apcraig commented May 20, 2021

I made a note for myself to review the documentation with regard to closed boundaries and close_boundaries. A quick look now suggests we can improve a bit. I'll include an update of the documentation with a future PR.

I think the confusion was mostly my doing. I probably wasn't careful enough when I was looking at this. Thanks.

apcraig added a commit to apcraig/CICE that referenced this issue Jun 19, 2021
- Fix bugs in history/restart frequency associated with new calendar (CICE-Consortium#589)
- Define frequency in absolute terms relative to 0000-01-01-00000 and document (CICE-Consortium#589)
- Update set_nml.histall to include hourly output (CICE-Consortium#589)
- Update test scripts to cleanly abort if run fails where possible (CICE-Consortium#608)
- Update decomp test so it's rerunable, remove restart at start of run (CICE-Consortium#601)
- Add ability to do bfbcomp tests with additional options set on command line (CICE-Consortium#569)
- Update documentation of calendar frequency computation, calendar types, and closed boundaries (CICE-Consortium#541)
- Add optional doabort flag to abort_ice to control whether the method aborts.  This
  is useful for testing and code coverage statistics, although doabort=.false.
  will not call the actual abort method, but we can test the interfaces and
  rest of the code.
apcraig added a commit that referenced this issue Jul 2, 2021
* Fix history/restart frequency bugs and update scripts

- Fix bugs in history/restart frequency associated with new calendar (#589)
- Update set_nml.histall to include hourly output (#589)
- Update test scripts to cleanly abort if run fails where possible (#608)
- Update decomp test so it's rerunable, remove restart at start of run (#601)
- Add ability to do bfbcomp tests with additional options set on command line (#569)
- Update documentation of calendar frequency computation, calendar types, and closed boundaries (#541)
- Add optional doabort flag to abort_ice to control whether the method aborts.  This
  is useful for testing and code coverage statistics, although doabort=.false.
  will not call the actual abort method, but we can test the interfaces and
  rest of the code.
- Add histfreq_base and dumpfreq_base ('init' or 'zero') to specify
  reference data for history and restart output.  Defaults are
  'zero' and 'init' respectively for hist and dump.
  Setting histfreq_base to 'zero' allows for consistent output
  across multiple runs.  Setting dumpfreq_base to 'init' allows
  the standard testing which requires restarts be written,
  for example, 5 days after the start of the run.
- Remove extra abort calls in bcstchk and sumchk on runs that
  complete fine but don't pass checks.  These aborts should never
  have been there.
- Update documentation.

- Clean up some of the unit tests to better support regression testing

- modify initial/restart implementation
- restart namelist is deprecated, now computed internally
- modify initial/continue init checks and set restart and use_restart_time as needed
- create compute_relative_elapsed method in ice_calendar to improve code reuse
- update documentation with regard to initial/continue modes
- Set default use_restart_time to false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants