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

no monthy or daily if ref_time is not '000000' #282

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

weiyuan-jiang
Copy link
Contributor

No description provided.

bring changes form develop
@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner August 12, 2020 13:59
Copy link
Contributor

@tclune tclune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really want a new library for this? The add_executable() macro allows you to specify multiple sources.

@gmao-rreichle
Copy link
Contributor

@tclune : The actual (2-dot) changes here are minimal. Not sure why you're getting messages. Perhaps something went wrong with Weiyuan's merge of develop into this branch.
There's no touching of CMake or anything else of note. We're just adding a check in the job file (lenkf.j) that verifies consistency of two HISTORY parameters.

@tclune
Copy link
Contributor

tclune commented Aug 12, 2020

Matt and I review all changes to the CMake files. This allows us to ensure consistency for the build system over time.

And there were changes to the CMakeLists.txt for the APP in this PR.

@weiyuan-jiang
Copy link
Contributor Author

The CMakeLists.txt changes are already in develop branch. This branch simply bring develop into it. So I believe the check is based on three dot comparison not two dot comparison.

@tclune
Copy link
Contributor

tclune commented Aug 12, 2020

GitHub clearly shows changes to CMake going into develop. Regardless, my comment is about whether you really want/need an extra library in the App directory. I'm going to approve now in the interest of speed. But please think about this.

@weiyuan-jiang
Copy link
Contributor Author

Yes, at this moment, we need an extra library in the App directory. But in the future, we may move all those preprocessing subroutines to a new location.

tclune
tclune previously approved these changes Aug 12, 2020
@gmao-rreichle
Copy link
Contributor

@tclune , @weiyuan-jiang , @mathomp4 :

At the risk of taking out of proportion... I'm still trying to understand what Tom's concern is here:

Do you really want a new library for this? The add_executable() macro allows you to specify multiple sources.

I'm guessing this refers to the following change:

image

This was approved previously by Tom and Matt in PR #283.

Would it be better to have preprocess_ldas_routines.F90 live in a different directory?
If so, where should it live?

It should be easy to make further changes. Note that we're about to make a full release of GEOSldas (after 8 beta releases), so now's a good time to make such a change.

@tclune
Copy link
Contributor

tclune commented Aug 12, 2020

Yes - that is the code in question, but the concern is different. And yes, I could have caught this change in the earlier PR, but sometimes I focus a bit more than others.

There is simply no need for the esma_add_library() command. The add_executable() command could simply absorb the new sourcefile. Delete lines 1-7 and change lines 20-24 to be:

   ecbuild_add_executable (	
      TARGET ${prog}.x 
      SOURCES ${prog}.F90	preprocess_ldas_routines.F90
      LIBS GEOSldas_GridComp mk_restarts)

We usually introduce libraries either because the source is needed in multiple executables or more commonly because the source is used by other layers of the model.

@tclune
Copy link
Contributor

tclune commented Aug 12, 2020

Ah - and it did not help that the GitHub diffs summary was hiding he loop over multiple executables. So there is some rationale for the library after all. One could still argue my change is simpler, but ... down in the weeds at this point.

Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addresses item 1.) of issue #204
Looks good to me. I approve assuming that it's been 0-diff tested.

@weiyuan-jiang weiyuan-jiang merged commit 1d2a8f6 into develop Aug 18, 2020
@weiyuan-jiang weiyuan-jiang deleted the check_hist_reftime branch September 2, 2020 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add consistency checks of HISTORY.rc selections
3 participants