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

Replacing crtm 2.3.0 with 2.4.0 + NCEP minor IR fix + Reconfigured Cloud detection for CrIS-FSR #30

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

gmao-wgu
Copy link
Contributor

@gmao-wgu gmao-wgu commented Mar 10, 2023

This PR was created on top of GEOSadas-5_29_5 with the updated CRTM from version 2.3.0 to 2.4.0 plus NCEP minor IR bug fix. gmao_global_satinfo.rc was also updated to reflect both reconfigured cloud detection for CrIS-FSR and reconfigured gross-check for AIRS, IASI and CrIS-FSR.

@gmao-wgu gmao-wgu requested a review from a team as a code owner March 10, 2023 01:09
@github-actions
Copy link

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found:

@gmao-wgu gmao-wgu added enhancement New feature or request 0 diff structural Structural changes to repository that are zero-diff Not 0-diff labels Mar 10, 2023
@gmao-wgu gmao-wgu changed the title replacing crtm 2.3.0 with 2.4.0 + NCEP minor IR fix + Reconfigured Cloud detection for CrIS-FSR Replacing crtm 2.3.0 with 2.4.0 + NCEP minor IR fix + Reconfigured Cloud detection for CrIS-FSR Mar 10, 2023
@mathomp4
Copy link
Member

@gmao-wgu Your labels are a bit confusing. Is the PR zero-diff or not? I'm imagining it isn't (lots of changes...)

@gmao-wgu gmao-wgu removed the 0 diff structural Structural changes to repository that are zero-diff label Mar 10, 2023
@gmao-wgu
Copy link
Contributor Author

@gmao-wgu Your labels are a bit confusing. Is the PR zero-diff or not? I'm imagining it isn't (lots of changes...)

I thought my PR did not cause any structural change, but not zero diff. My bad.

Copy link
Collaborator

@rtodling rtodling left a comment

Choose a reason for hiding this comment

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

Wei, there is only one thing that needs a little clarification:

I think the CMakeLists as you have it now bypasses fixes that Matt put in to address the occasional issue w/ compilation error of CRTM_module ... check diff at the bottom of CMakeLists

Perhaps Matt can help looking over this.

Also, the top few lines of the CMake have been commented out with a comment stating no understanding for the reasons those lines were there ... I think here too, Matt might be able to clarify.

@mathomp4
Copy link
Member

Wei, there is only one thing that needs a little clarification:

I think the CMakeLists as you have it now bypasses fixes that Matt put in to address the occasional issue w/ compilation error of CRTM_module ... check diff at the bottom of CMakeLists

Perhaps Matt can help looking over this.

@gmao-wgu I agree with @rtodling It's possible ye olde ADA_Module.F90 no longer has an issue with debug, but until we can try it out, you should make the end of the CMakeLists look like:

if (CMAKE_Fortran_COMPILER_ID MATCHES Intel)
  set (CMAKE_Fortran_FLAGS_RELEASE "-free ${FOPT2} ${LITTLE_ENDIAN} ${BYTERECLEN}")
  if (CMAKE_BUILD_TYPE MATCHES Debug)
    message(WARNING "Intel Compiler and our debugging flags have issues with ADA_Module.f90. So for now we turn off checking compiling ADA_Module.f90")
    set_source_files_properties(ADA_Module.f90 PROPERTIES COMPILE_OPTIONS -nocheck)
  endif ()
endif ()

This code just says "If Intel and Debug, compile ADA_Module.F90 with no check flags"

Also, the top few lines of the CMake have been commented out with a comment stating no understanding for the reasons those lines were there ... I think here too, Matt might be able to clarify.

These lines:

# no idea why this is not a Fortran file?
#add_custom_command (
#  OUTPUT CRTM_Module.F90
#  COMMAND cp ${CMAKE_CURRENT_SOURCE_DIR}/CRTM_Module.fpp CRTM_Module.F90
#  WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
#  )

should just be deleted. It looks like in CTRM 2.4.0 they are no longer using CRTM_Module.fpp so this bit of CMake is redundant.

@rtodling
Copy link
Collaborator

Thanks for the input Matt.

Wei: can you add make the changes as suggested by Matt, and commit them? thanks.

@mathomp4
Copy link
Member

Thanks for the input Matt.

Wei: can you add make the changes as suggested by Matt, and commit them? thanks.

I could do it if you like? Would be pretty quick for me to do so.

@gmao-wgu
Copy link
Contributor Author

Thanks for the input Matt.
Wei: can you add make the changes as suggested by Matt, and commit them? thanks.

I could do it if you like? Would be pretty quick for me to do so.

Matt, I am sorry I could not do this today. Of course, go ahead. Thanks!

@mathomp4
Copy link
Member

Thanks for the input Matt.
Wei: can you add make the changes as suggested by Matt, and commit them? thanks.

I could do it if you like? Would be pretty quick for me to do so.

Matt, I am sorry I could not do this today. Of course, go ahead. Thanks!

Done.

@gmao-wgu
Copy link
Contributor Author

Thanks for the input Matt.

Wei: can you add make the changes as suggested by Matt, and commit them? thanks.

Ricardo, Today is Jedi Wednesday, I could do it tomorrow or this evening. Thanks.

@rtodling
Copy link
Collaborator

I just gave this a try and this looks fine with me.

@rtodling
Copy link
Collaborator

Matt: I don't have review powers of NCEP_Shared - I wish I had ... can you pls approve and promote this to develop/main?
Thanks

Copy link
Collaborator

@rtodling rtodling left a comment

Choose a reason for hiding this comment

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

All good by me - run just fine for me.

Copy link
Member

@mathomp4 mathomp4 left a comment

Choose a reason for hiding this comment

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

Approve for CMake

@mathomp4
Copy link
Member

Matt: I don't have review powers of NCEP_Shared - I wish I had ... can you pls approve and promote this to develop/main? Thanks

@rtodling Sure. I'll merge in. Would you like a new release as well?

@mathomp4 mathomp4 merged commit 5dd56af into main Mar 22, 2023
@mathomp4
Copy link
Member

Matt: I don't have review powers of NCEP_Shared - I wish I had ... can you pls approve and promote this to develop/main? Thanks

@rtodling Sure. I'll merge in. Would you like a new release as well?

@rtodling I made a release here: https://github.com/GEOS-ESM/NCEP_Shared/releases/tag/v1.3.0

Just to be safe. If you'd prefer this as v2.0.0, just let me know and I can remake it. (All depends on how you'd like to number it!)

@rtodling
Copy link
Collaborator

Matt: thank you. Can you make this version v1.3.0 for NCEP_Shared?

@mathomp4
Copy link
Member

Matt: thank you. Can you make this version v1.3.0 for NCEP_Shared?

It is there: https://github.com/GEOS-ESM/NCEP_Shared/releases/tag/v1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Not 0-diff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants