-
Notifications
You must be signed in to change notification settings - Fork 132
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
RASM test update, new bathymetry popfile method, minor refactor to ice_dyn_evp_1d #367
Conversation
…s. Add get_bathymetry_popfile from RASM
@phil-blain, my hope is this addresses #366. I think it's worth a quick test on your end with the nemo3.6 system. If other changes are needed and they are not extensive, maybe we can include them in this PR. If it looks like more work to get CICE working with nemo3.6, then maybe another PR is needed. We can decide once we have more info. thanks! |
@duvivier, it looks like the read-the-docs is failing due to a python version issue. Does this make any sense to you? If I login to readthedocs, it looks like this failure started at least 3 days ago and is not related to the current PR in particular. It looks like it's something to do with bibtex, but I could be wrong. |
regarding readthedocs issues, maybe we need to add a configuration file and shift to v2? https://docs.readthedocs.io/en/stable/config-file/index.html |
@apcraig thanks for bringing this up. It's irritating that it worked just fine last week and now something has changed on that end that's making it fail. Let me look into it a bit. I'll try just a normal build on the master branch first to see if that's broken too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I tried it with NEMO and it works (it can compile these files correctly). I did not get to the end of the build though because there is some other code in NEMO and our in house file I/O that needs to be adapted for dynamic allocation in CICE6, so some more changes might be needed later.
My only comment would be why the header section of the dmi_omp and bench_v2 modules were kept but commented. I feel it's better when we do these kind of changes to completely get rid of old code, because commented code can be confusing later on.
@phil-blain, glad to hear we've addressed at least one of the nemo build issues. We can make other changes if needed in the future. I can clean up the old commented out code. This code was provided by DMI and in these cases, I sometimes think it's nice to keep the old code around to see the changes and maybe compare to any updates on their end moving forward. I don't know if that's helpful in this case or not. You're probably right that I should maybe clean that up. |
thanks @duvivier. I'm happy to help do some googling/testing/etc if you hit a wall or have other high priority tasks. Just let me know. |
@apcraig sorry I've been slow. I just had other meetings/telecons today that have hampered focusing on this. I will try tonight or tomorrow, but if that's holding things up please feel free to google etc. Sorry. |
@duvivier, no worries, no rush. Thanks for having a look! |
I know there is build the docs problems, but the code looks good to me. |
@apcraig I have determined this is an issue in general with all our documentation (not just this PR). I'm starting a new development branch to try and figure it out. |
Thanks @duvivier! I am quite confident that this PR is not introducing a problem, it broader. At this point, I think we should ignore the failing readthedocs check and let this (and other work) move forward at its pace. We will fix the readthedocs as a separate PR. I guess we have to fix the readthedocs before we release, so that's really the target we should be thinking about. Thanks again, keep me posted. |
@apcraig I think I solved it. It had to do with the settings in the consortium RTD. Under "Advanced Settings" there is a python interpreter choice. We had Python 2.x chosen, but I just switched it to 3.x and it seems to work now. In requirements.txt we call sphinxcontrib-bibtex. We don't specify a version, so it just calls the latest version. I just checked and it appears they had a release on Sept.20. My guess is that the newest release stopped support for python v.2 and started support for python v.3. |
@apcraig I am trying to figure out how to re-trigger the documentation build for this PR in RTD, but I can't figure it out. At any rate, I think you should go ahead and merge this into the master and then the master documentation should build properly with the change I introduced in the RTD settings for the consortium. |
@duvivier Excellent, thank you! I am looking to retrigger the build too. I may just push a small change to PR if I can't figure it out either. |
@apcraig This looks like it did the trick for the RTD tests. |
I cleaned up the old code as suggested by @phil-blain, that triggered new builds and readthedocs is passing again! Thanks @duvivier |
The travis build is failing due to the NCAR ftp download. For future reference, another way to trigger a new build is by pushing an empty commit : |
@phil-blain It looks like you (or @apcraig ) retriggered the travis build. Nothing has changed on the NCAR side, so I'm not sure why this occasionally comes up. I will look into moving the ftp data as we've discussed but not till after the next release. Also the JRA55 data nearly doubled the size, so that may become an issue too. |
The travis build can be retriggered manually and I have done that. The readthedocs doesn't seem to have that capability yet. Good to know about the empty git push option. I will merge this later today unless I hear other concerns. |
PR checklist
Test in RASM and update
apcraig
Tested on izumi, 4 compilers vs current master,
#c21d34b37 at https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks
Merge three ice_dyn_evp_1d modules to one module and update ice_dyn_evp_1d interface names.
Add get_bathymetry_popfile from RASM, turned off by default
Fix circular logic issue that only was created with CESMCOUPLED turned on