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

CLM & Mosart fixes for PIO2 #2772

Merged
merged 2 commits into from
Mar 18, 2019
Merged

CLM & Mosart fixes for PIO2 #2772

merged 2 commits into from
Mar 18, 2019

Conversation

jayeshkrishna
Copy link
Contributor

@jayeshkrishna jayeshkrishna commented Feb 28, 2019 •
edited by bishtgautam
Loading

This PR includes some fixes in CLM and Mosart required for PIO2.

Although these fixes are not specific to PIO2, they showed up during testing with PIO2.

Fixes #2778
[BFB]

@dqwu @jayeshkrishna
Commenting out LND_PIO_REARRANGER entry in config_pio.xml for E3SM
679a1c8 
This setting causes LND to use a hard-coded value of 1 for I case
and F case.
@jayeshkrishna jayeshkrishna self-assigned this Feb 28, 2019
@jayeshkrishna
Copy link
Contributor Author

jayeshkrishna commented Feb 28, 2019 •
edited
Loading

I tested this branch with a successful run of CLM+Mosart case (f19_g16 + I1850CLM45 + PIO1) for 5 days on Cori.

These changes have been more extensively tested in the dqwu/test_pio2_03 branch that has been used for several E3SM tests (developer test suite, integration test suite, high res runs etc)

@jayeshkrishna jayeshkrishna requested a review from dqwu February 28, 2019 20:00
@bishtgautam
Copy link
Contributor

Are the changes in this PR compatible with PIO1?

@jayeshkrishna
Copy link
Contributor Author

Yes, all the changes are compatible with PIO1 (I did the testing above with PIO1).

dqwu
dqwu approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@dqwu dqwu left a comment

Choose a reason for hiding this comment

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

I am OK with these minor changes. I will test this branch on anlworkstation.

@bishtgautam
Copy link
Contributor

@jayeshkrishna, It might be due to my ignores about PIO2, but I don't understand why these changes are being made.

@jayeshkrishna
Copy link
Contributor Author

These are bugs in the code that showed up when testing with PIO2. CLM was providing invalid starts/counts to the PIO library and this commit fixes that issue.
With PIO1 the entire library is implemented in fortran and since fortran arrays always include the size with it these bugs did not show up (inadvertently). When testing with PIO2 these bugs showed up.
The changed code works fine with PIO1 and PIO2.

@jayeshkrishna
Copy link
Contributor Author

Note that the code provided starts/counts to PIO corresponding to the time dimension even when it was not present.

@bishtgautam
Copy link
Contributor

@jayeshkrishna, Thanks for the explanation. I created an issue based on your comment.

bishtgautam
bishtgautam requested changes Mar 6, 2019
View reviewed changes
Copy link
Contributor

@bishtgautam bishtgautam left a comment

Choose a reason for hiding this comment

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

Instead of leaving the incorrect code as a comment, how about removing it?

cime/config/e3sm/machines/config_pio.xml
@@ -123,13 +123,13 @@
</values>
</entry>
-->
<entry id="LND_PIO_REARRANGER">
<!-- <entry id="LND_PIO_REARRANGER">
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing these entries instead of leaving them as comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be useful to retain this entry to show how to use it in future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

components/mosart/src/riverroute/RtmIO.F90 Outdated
@@ -1095,8 +1097,8 @@ subroutine ncd_io_int_var1_nf(varname, data, flag, ncid, readvar, nt)
else
start(1) = 1
count(1) = size(data)
start(2) = 1
count(2) = 1
!start(2) = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing these entries instead of leaving them as comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will remove the commented code here and rebase the branch.

components/mosart/src/riverroute/RtmIO.F90 Outdated
@@ -1156,8 +1160,8 @@ subroutine ncd_io_log_var1_nf(varname, data, flag, ncid, readvar, nt)
else
start(1) = 1
count(1) = size(data)
start(2) = 1
count(2) = 1
!start(2) = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing these entries instead of leaving them as comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will remove the commented code here and rebase the branch.

components/mosart/src/riverroute/RtmIO.F90 Outdated
if (present(nt)) then
start(1) = 1
start(2) = nt
count(1) = size(data)
count(2) = 1
else
start(1) = 1
start(2) = 1
!start(2) = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing these entries instead of leaving them as comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will remove the commented code here and rebase the branch.

components/mosart/src/riverroute/RtmIO.F90 Outdated
@@ -1335,10 +1343,10 @@ subroutine ncd_io_int_var2_nf(varname, data, flag, ncid, readvar, nt)
else
start(1) = 1
start(2) = 1
start(3) = 1
!start(3) = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing these entries instead of leaving them as comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will remove the commented code here and rebase the branch.

components/mosart/src/riverroute/RtmIO.F90 Outdated
@@ -1394,10 +1404,10 @@ subroutine ncd_io_real_var2_nf(varname, data, flag, ncid, readvar, nt)
else
start(1) = 1
start(2) = 1
start(3) = 1
!start(3) = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing these entries instead of leaving them as comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will remove the commented code here and rebase the branch.

@jayeshkrishna
Fixing starts/counts for pio puts
09061d0 
The starts/counts for puts where the vars (1d/2d/3d) had no time
dimensions were wrong, i.e., always had an extra start/count
corresponding to the time dimension. Now we have the starts/counts
for time dimension only when the var is supposed to have
multiple timesteps
@jayeshkrishna jayeshkrishna force-pushed the jayeshkrishna/pio2_clm_mosart_fixes branch from 2bfba79 to 09061d0 Compare March 6, 2019 16:26
bishtgautam
bishtgautam approved these changes Mar 6, 2019
View reviewed changes
Copy link
Contributor

@bishtgautam bishtgautam left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

@jayeshkrishna
Copy link
Contributor Author

The branch is now rebased with the changes requested.

jayeshkrishna added a commit that referenced this pull request Mar 15, 2019
@jayeshkrishna
Merge branch 'jayeshkrishna/pio2_clm_mosart_fixes' into next (PR #2772)
ad9edaa 
This PR includes some fixes in CLM and Mosart required for PIO2.

Although these fixes are not specific to PIO2, they showed up
during testing with PIO2.

Fixes #2778
[BFB]

* jayeshkrishna/pio2_clm_mosart_fixes:
  Fixing starts/counts for pio puts
  Commenting out LND_PIO_REARRANGER entry in config_pio.xml for E3SM
@jayeshkrishna jayeshkrishna merged commit 4c4aeff into master Mar 18, 2019
jgfouca pushed a commit that referenced this pull request Jun 25, 2019
@jayeshkrishna
Merge pull request #2772 from E3SM-Project/jayeshkrishna/pio2_clm_mos…
802bbc4 
…art_fixes

This PR includes some fixes in CLM and Mosart required for PIO2.

Although these fixes are not specific to PIO2, they showed up during testing with PIO2.

Fixes #2778
[BFB]
@dqwu dqwu mentioned this pull request Sep 9, 2019
Closed
dqwu added a commit that referenced this pull request Sep 9, 2019
@dqwu
Refixing starts/counts for pio puts
b58e384 
The starts/counts for puts where the vars (1d/2d/3d) had no time
dimensions were wrong, i.e., always had an extra start/count
corresponding to the time dimension. Now we have the starts/counts
for time dimension only when the var is supposed to have
multiple timesteps.

This fix (part of PR #2772) was accidentally reverted by PR #3012.
@dqwu dqwu mentioned this pull request Sep 9, 2019
Merged
bishtgautam added a commit that referenced this pull request Sep 11, 2019
@bishtgautam
Merge branch 'dqwu/scorpio_mosart_fixes' into next (PR #3178)
5eaa7c0 
This PR includes some fixes in Mosart required for scorpio (PIO2).

These fixes (reverted by PR #3012) are re-picked from PR #2772.
They are not specific to scorpio, but the bugs showed up during
testing with scorpio. They should be tested with scorpio_classic
as well.

Fixes #3176
[BFB]
bishtgautam added a commit that referenced this pull request Sep 12, 2019
@bishtgautam
Merge branch 'dqwu/scorpio_mosart_fixes' (PR #3178)
eff0f2e 
This PR includes some fixes in Mosart required for scorpio (PIO2).

These fixes (reverted by PR #3012) are re-picked from PR #2772.
They are not specific to scorpio, but the bugs showed up during
testing with scorpio. They should be tested with scorpio_classic
as well.

Fixes #3176
[BFB]
bartgol pushed a commit that referenced this pull request Mar 27, 2024
@E3SM-Bot
Merge Pull Request #2772 from E3SM-Project/scream/bartgol/eamxx/fix-f…
53561df 
…lushing-of-output-files

Automatically Merged using E3SM Pull Request AutoTester
PR Title: Fix small bug in flushing file in scorpio interface
PR Author: bartgol
PR LABELS: I/O, AT: AUTOMERGE, bugfix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bishtgautam bishtgautam bishtgautam approved these changes

@dqwu dqwu dqwu approved these changes

Assignees

@jayeshkrishna jayeshkrishna

Labels
BFB PR leaves answers BFB bug fix PR Land PIO River
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CLM providing invalid starts/counts to the PIO
3 participants
@jayeshkrishna @bishtgautam @dqwu