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

Feature/wjiang/combine easev1 v2 #639

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Sep 12, 2022

OBSOLETE! Addressed with #634.

Combine easev1 and v2. Some public interfaces in EASE_conv.F90 will be removed after mkSMAPTilesPara.F90 is removed.

@weiyuan-jiang weiyuan-jiang added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup labels Sep 12, 2022
@weiyuan-jiang weiyuan-jiang requested review from a team as code owners September 12, 2022 14:14
@weiyuan-jiang
Copy link
Contributor Author

weiyuan-jiang commented Sep 12, 2022

Obviously a typo here. It is v1, so it should be just 'EASE' . It will lead to non-zero diff in the future when it is corrected.

Copy link
Collaborator

@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.

CMake changes ok

@gmao-rreichle
Copy link
Contributor

Obviously a typo here. It is v1, so it should be just 'EASE' . It will lead to non-zero diff in the future when it is corrected.

This correction is fine. It's only changing a string in a file header and should be easily identifiable as a bug fix. My understanding is that the current branch already fixes this typo.

@gmao-rreichle
Copy link
Contributor

@weiyuan-jiang, a note on my latest commit: 6e467b4
Before I made this commit, I approached this PR in isolation and did not refer back to the make_bcs cleanup project, and I promptly forgot that we have LDAS_EASE_conv.F90 in the GEOSldas repo. I now realize that your new EASE_conv.F90 was just a copy of LDAS_EASE_conv.F90. (It would have been very helpful if this equivalence had been pointed out in the commit messages or in the PR comments.)
In any case, I'm not 100% sure that LDAS_EASE_conv.F90 (or, equivalently, EASE_conv.F90 before my latest commit) is 0-diff against easeV1_conv.F90 because of the difference in the definition of PI for the EASEv1 case. This should be fixed in my latest commit, along with some minor cleanup of comments and clarification of global constants. We'll need to decide which version of EASE_conv.F90 is best going forward.

@gmao-rreichle
Copy link
Contributor

Two more comments on mkSMAPTilesPara_v2.F90:

  1. I'm confused what subroutine mkEASEv2Raster() does, and why this is not done for EASEv1.
  2. Also, subroutine write_tilfile() seems obsolete.

@gmao-rreichle
Copy link
Contributor

Two more comments on mkSMAPTilesPara_v2.F90:

  1. I'm confused what subroutine mkEASEv2Raster() does, and why this is not done for EASEv1.
  2. Also, subroutine write_tilfile() seems obsolete.

These subroutines are not needed right now but were probably needed for the irrigation bcs processing. For now, comment out the subroutines.

@weiyuan-jiang
Copy link
Contributor Author

Do not comment on this PR. It will be replaced by #634

@gmao-rreichle
Copy link
Contributor

OBSOLETE! Addressed with #634. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch. cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants