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.

Sign up for GitHub

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

Jump to bottom

allow processors without tiles (N_catl=0) #309

Merged
merged 6 commits into from
Sep 17, 2020
Merged

allow processors without tiles (N_catl=0) #309

merged 6 commits into from
Sep 17, 2020

Conversation

weiyuan-jiang
Copy link
Contributor

If a process has no tile, it crashes when running data assimilation. Is it safe to assume N_obsl = 0 if N_catl =0 ? @gmao-rreichle

@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner September 11, 2020 14:55
@gmao-rreichle
some fixes to previous commit:
f7fdb61 
- keep processing for optional variable "obsbias_ok" (would otherwise need to edit calls to get_obs_pred() in clsm_ensupd_enkf_update.F90)
- added "(1:kk)" to vectors call to dist_km2deg()
- moved cycle statements to beginning of loop through processors (3 occurrences)
- comments and whitespace modifications
@gmao-rreichle
Copy link
Contributor

@weiyuan-jiang : I added a few changes, see commit message f7fdb61

I did not test (or even compile) the changes. Please review and let me know if you have any questions.

Is it safe to assume N_obsl = 0 if N_catl =0 ?

That should be safe. If there are no tiles on a given processor, then there cannot be obs assigned to tiles on that processor.

@weiyuan-jiang weiyuan-jiang changed the title hotfix for not tiles in a process hotfix for no tiles in a process Sep 16, 2020
weiyuan-jiang and others added 2 commits September 16, 2020 11:50
@weiyuan-jiang
fix for obsbias_ok_tmp
fd01a05
@gmao-rreichle
remove "Nobs_l>0" condition from handling optional argument "obsbias_ok"
ada3d60 
to preserve symmetry with the subsequent handling of optional argument
"fcsterr_inflation_fac"
@gmao-rreichle gmao-rreichle changed the title hotfix for no tiles in a process allow processors without tiles (N_catl=0) Sep 16, 2020
@gmao-rreichle
Copy link
Contributor

@weiyuan-jiang : I agree with the change of "obsbias_ok" to "obsbias_ok_tmp" late in subroutine get_obs_pred().
I reverted to the original code for handling the optional argument "obsbias_ok" (ie, I removed the "Nobs_l>0" condition.
The condition would also apply to the processing of the subsequent optional argument ("fcsterr_inflation_fac"), and probably many other statements throughout the subroutine. At this time, I think it's best to apply the "Nobs_l>0" (or equivalent) condition only where it is necessary to avoid giving the impression that the condition was applied consistently throughout the subroutine.
I went ahead and re-approved the pull request. As far as I am concerned, it is ready for merging.

@biljanaorescanin

gmao-rreichle
gmao-rreichle previously approved these changes Sep 16, 2020
View reviewed changes
@weiyuan-jiang
Copy link
Contributor Author

If N_obsl =0, the assignment obsbias_ok_tmp = .false. will crash. @gmao-rreichle

@gmao-rreichle
Copy link
Contributor

Of course, sorry! Thanks for catching this, @weiyuan-jiang
I re-instated the condition "N_obsl>0".
Should now be good to go. Or so I hope...

@gmao-rreichle
Copy link
Contributor

Successful 0-diff tests by @biljanaorescanin completed 16 Sep 2020.

@biljanaorescanin biljanaorescanin merged commit 2fccefe into develop Sep 17, 2020
@biljanaorescanin biljanaorescanin deleted the hotfix/no_tile_for_assim branch September 17, 2020 12:20
biljanaorescanin added a commit that referenced this pull request Oct 13, 2020
@biljanaorescanin
Revert "Allow processors without tiles (N_catl=0) (#309)"
9451086 
This reverts commit 2fccefe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmao-rreichle gmao-rreichle gmao-rreichle approved these changes

Assignees
No one assigned
Labels
0-diff bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@weiyuan-jiang @gmao-rreichle @biljanaorescanin