-
Notifications
You must be signed in to change notification settings - Fork 108
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
Merge to develop #45
Merge to develop #45
Conversation
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 ok to me. Are these the same changes that @GeorgeGayno-NOAA made to the master/develop branch? If so, I am happy to merge them right away.
sorc/chgres_cube.fd/input_data.F90
Outdated
@@ -172,126 +172,126 @@ subroutine read_input_nst_data(localpet) | |||
c_d_input_grid = ESMF_FieldCreate(input_grid, & | |||
typekind=ESMF_TYPEKIND_R8, & | |||
staggerloc=ESMF_STAGGERLOC_CENTER, rc=rc) | |||
if(ESMF_logFoundError(rcToCheck=rc,msg=ESMF_LOGERR_PASSTHRU,line=__LINE__,file=__FILE__)) & |
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.
@jedwards4b @GeorgeGayno-NOAA I see Jim removed those changes from the PR to ufs_release_v1.0. This is good, because the code that was reintroduced by commit 4b91f33 is not correct and incompatible with the GNU compilers. CPP directives have to be uppercase, i.e.
if(ESMF_logFoundError(rcToCheck=rc,msg=ESMF_LOGERR_PASSTHRU,line=__LINE__,file=__FILE__))
is correct but
if(ESMF_logFoundError(rcToCheck=rc,msg=ESMF_LOGERR_PASSTHRU,line=__line__,file=__file__))
is not. @GeorgeGayno-NOAA can you please revert this change for the develop branch?
Sorry. I am a bit confused about what you are doing. Some of these changes (zeroing out 'w') are already in 'develop'. You should be merging from 'develop' to all your various branches. |
@GeorgeGayno-NOAA I had to cherry pick the develop branch to get only the changes that I need, as @climbfuji points out the change eg from |
'develop' is the main line of development. I assume all changes to the release/ufs_release_v1.0 will get pushed back to 'develop' at some point. Therefore, the release branch must be kept up-to-date with 'develop'. If the merge from 'develop' results in some issues, then you can make a subsequent change to your branch and commit it. |
I am sorry but I disagree. If a bug is introduced in develop then it should be fixed there, not in any other branch that eventually gets merged back. Of course it should never have passed the code review in the first place, but these things can happen. I can create the PR if needed. |
@climbfuji - Hang on, I can try the merge again. |
890faeb
to
82f279c
Compare
@climbfuji @GeorgeGayno-NOAA I redid the merge. I'm not sure about all of those deleted files. |
The deleted files look ok to me, 90% of them are for a system called theia that has been decommissioned a few months back. |
@@ -1,4 +1,4 @@ | |||
[submodule "cmake"] | |||
path = cmake | |||
url = https://github.com/NOAA-EMC/CMakeModules.git | |||
branch = ufs_release_1.0 | |||
branch = ufs_release_v1.0 |
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.
Weird. I remember having fixed this in the past.
I am not sure where the bug is. I just checked out 'develop' (commit 732210a) on the machines that are currently supported - Jet, Hera, Cray and Dell. chgres_cube built with no problems and the regression tests passed. |
I assume you were using the Intel compiler, for which you won't see the problem. If you use GNU, for example, the compilation will fail. |
I assume that you did not use cmake to do these tests either, it would be good for this project if you added the cmake build and tested it in develop. |
These changes are nessasary for the ufs_release branch cime debug tests to pass.