Isotopes for Icepack #307
Isotopes for Icepack #307
Conversation
demonstrate prototype optional argument handling internal to the columnphysics migrate to fortran use for n_iso and tr_iso, remove them from interfaces (to be done to other similar variables) fix isotope hardcoded basal ice growth and isotope update logic refactor optional closing argument to follow new approach (get ride of "extra" ridge_ice call) fix isotope restart bug (read unit number) add isotope restart test to base suite add subnames to icepack_isotope.F90 rename public icepack_isotope variable frac to isotope_frac_method to improve clarity add some coding standard documentation to developers guide under column physics section
Clean up Icepack interface for isotopes
|
I'm happy to do some additional testing when it's appropriate. Just let me know how I can help. It sounds like the first step might be review and testing of the isotope implementation by @dabail10. Then when that is ready, some further technical testing with full test suites on multiple machines/compilers? If that's not the best process, let me know. I assume this goes for both the Icepack and CICE mods. |
|
Yes, both codes need review and testing. I'm trying to test them both against the master baselines now, to see what still needs work besides cice restarts. Here's my plan:
@dabail10 please take a look at the code at any point in this process. It's already producing what I think are reasonable looking results, but it definitely needs your experienced eyeballs. @apcraig if you'd like to jump in with the regression testing, I'd be grateful! |
|
Sounds good, I will start some of the regression testing on multiple machines/compilers as well. |
|
Sorry I haven't had time to look at this. I will put aside a couple hours tomorrow on this. Dave |
|
Icepack v1.2.1 baseline: regression test of iiso2 against v121 with isotopes turned off: |
|
Full test suites on conrad with 4 compilers are here (#b408817a). What you see is that everything passes fine except 7 intel tests are failing regression tests. This is the same result as tests from about a month ago, and I attribute this to compiler optimization with intel. Other compilers are bit-for-bit and all intel debug tests are bit-for-bit. We could look into this further, but I'm not convinced it's needed. I am just running the test suite out of the box. |
There was a problem hiding this comment.
I've run your iiso2 branch and get what I expected for the isotopes in the sense that it is still not conserving. I have some ideas here, but I think these are the basic code mods needed to get isotopes in. I know you are still working on some of the indexing stuff, but the "science" changes look good to me.
|
Great, thanks @dabail10. I've done as much with the indexing as I plan to, for the isotopes PRs. We can continue to work on that separately. We also can merge this as it is and make an issue to track down the conservation error later. |
|
I am running a CICE case now with history output to verify the science changes there. These will have to be merged together. |
|
We should probably close #275 when it's appropriate. |
|
The documentation needs to be updated before this is merged. @dabail10 you can submit a PR to my fork, or make the changes in your fork and I'll grab them. We'll need this for CICE too. |
|
I think this PR is ready to merge, but the readthedocs check isn't finished (after 3+ hours). Is it possible to relaunch that? |
|
I changed the python version in my readthedocs account from 2.x to 3.x, and then the build succeeded. I didn't see a way to relaunch buildthedocs from here, so I had to commit a minor change in one of the files (which needed to be done anyhow), but that means that I'm not 100% sure whether the change to 3.x is what fixed the build problem here - I'm 99% sure. I guess I shouldn't be surprised that a PR would use the settings in the user's account rather than the settings for the main repository being merged into. Regardless, we might want to change the documentation workflow to tell people to use 3.x instead of 2.x. This PR is ready to merge. |
|
@eclare108213, I had a quick look around and think everything is fine. Why don't you merge this PR when you're ready. |
|
I ran full test suites on several platforms today with the current master. https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash#1ae044604498b8d268df6c577556d22d2baa7758. Results are as expected. |
* Move makdep compilation to Makefile * makdep compiled from a makdep rule in the Makefile * Add CFLAG_HOST macro * makdep is only compiled if needed * This addresses CICE-Consortium#306 * Use $(DEPGEN) instead of makdep as target (Makefile) * Update documentation
This PR is not yet ready to be merged - further review, testing and documentation is required.
Addition of isotope diagnostics following @dabail10's implementation for CESM.
@dabail10 @eclare108213 @apcraig
Initial tests by @apcraig looked okay
(see Iso1 interface eclare108213/Icepack#1)
but this needs to be reconfirmed.
Documentation for the interfaces is currently here but the isotope physics and code description needs to be added.
I will add the associated CICE PR shortly. Both this PR and the CICE one need @dabail10 's scrutiny to make sure the results are correct.