Add mostRecentAccessTime attribute to streams

[trhille]

Add a new attribute to streams called 'mostRecentAccessTime', which is useful when multiple time-levels are needed at once from an input file.

For input, this corresponds to the time for which input was last read. For output, this corresponds to the time for which output was last written. Note that for output, there is an option to specify an output time that is not the current time; the value used here for mostRecentAccessTime uses the time actually sent to the file.

The actualWhen argument to the MPAS_stream_mgr_read() routine is replaced with an optional argument saveActualWhen. The actualWhen argument was ambiguous when MPAS_stream_mgr_read was called in a way such that multiple streams were read; the actualWhen value returned came from the final stream read and could possibly be wrong for the other streams that were read.

Now, instead of being able to have the actualTime returned as an argument, there is the option to have the actual time saved to the new attribute. It can then be retrieved from the stream cursor as needed.

The saveActualWhen argument is optional because some streams do not contain xtime and trying to obtain ‘actual when’ in those cases will cause the reading of the stream to fail. Therefore, it is important to be able to not seek this information in these cases. If the saveActualWhen argument is not provided to MPAS_stream_mgr_read(), its value defaults to false.

This also changes the land ice core to stop using actualWhen and replace it with saveActualWhen. Neither the ocean nor seaice cores use actualWhen so no other changes are necessary.

[BFB]

[matthewhoffman]

@mgduda , you may be interested in this small MPAS Framework PR.

[xylar]

@trhille, could you put the full commit messages (possibly lightly edited) as the description of this PR? I think all that info is necessary to understand what's going on here.

What kind of testing would you suggest to make sure this works as expected? I can certainly run some COMPASS and standard E3SM test cases.

[trhille]

@trhille, could you put the full commit messages (possibly lightly edited) as the description of this PR? I think all that info is necessary to understand what's going on here.

Thanks @xylar, I've added this to the PR description.

What kind of testing would you suggest to make sure this works as expected? I can certainly run some COMPASS and standard E3SM test cases.

I think what we need is to write out xtime from the two most recent time levels to a log file, but I recognize that this is sort of a non-standard test requiring some extra code that wouldn't be part of the PR. @matthewhoffman, do you have other ideas for testing?

[matthewhoffman]

Thanks, @trhille . For testing, can you report in a comment the testing you have done in your PR to confirm the new functionality works as expected? (and reference your PR in our repo as an example of usage)

Because the ocean and sea ice cores do not use the functionality being modified, no specific testing needs to be done by them, but a standard set of COMPASS tests to confirm no harm was done is still appropriate. I think that would be sufficient.

[mgduda]

Thanks for tagging me, @matthewhoffman . Some of our code in MPAS-Atmosphere does make use of the actualWhen argument, which is removed in this PR. I'm not yet sure whether we'll make these changes in the MPAS-Model repository, so my opinion shouldn't carry much weight. But in cases where we don't explicitly iterate over streams with stream_cursor in user code, how would we retrieve mostRecentAccessTime in those cases? For example: https://github.com/MPAS-Dev/MPAS-Model/blob/v7.1/src/core_atmosphere/mpas_atm_core.F#L643-L644 .

[xylar]

@mgduda, from my reading of the changes in this PR and @matthewhoffman's description in a meeting we were in earlier today, it seems like actualWhen gives the most recent xtime read from the last of the streams in the stream manager. It seems like this could be a pretty odd behavior in any situation where you are reading multiple streams that may have different xtime intervals in them. So it doesn't seem like the code as written is super safe in the sense of giving a user a reliable value for actualWhen when reading multiple streams.

[mgduda]

@xylar Do you mean reading multiple streams with a single call to mpas_stream_mgr_read, or reading multiple streams, each with its own call to mpas_stream_mgr_read? In my experience, the actualWhen value is absolutely reliable in the latter case, and I don't see how to recover this existing functionality under the changes in this PR.

[xylar]

@mgduda, I'm not well versed enough in the I/O framework to address your question. My sense it that @matthewhoffman was referring to the former case. I would guess for the latter case, you would want, indeed, need to "iterate" over the one stream with a stream_cursor to get the attribute you want, which might lead to clumsier code than you'd like.

My understanding is that MPAS-Ocean and MPAS-Seaice don't use this functionality, so I'll leave further discussion up to the MALI and MPAS-Atmosphere folks.

[trhille]

Thanks, @trhille . For testing, can you report in a comment the testing you have done in your PR to confirm the new functionality works as expected? (and reference your PR in our repo as an example of usage)

I tested this for MALI on https://github.com/trhille/E3SM/tree/0d1f47742a394d3fb24fc6c9c72f837d86419562. I wrote out xtime of the two most recent input times here: https://github.com/trhille/E3SM/blob/0d1f47742a394d3fb24fc6c9c72f837d86419562/components/mpas-albany-landice/src/mode_forward/mpas_li_calving.F#L1809-L1829

The input stream interval is one year (updated in July) in this case. This gave the expected behavior in log.landice.0000.out:

   Completed timestep.  New time is: 2094-07-01_00:00:00
 Starting timestep number 5
   Setting time step (days) to: 92.6880439814815
 Calculating N assuming perfect ocean connection.
   * Forced a read of input stream 'ismip-gis' from time: 2094-07-01_00:00:00
   * Forced a read of input stream 'ismip-gis' from time: 2093-07-01_00:00:00
 deltatForcing = 31536000.0000000
...
   Completed timestep.  New time is: 2095-04-04_01:23:06
 Starting timestep number 8
   Setting time step (days) to: 87.9422916666667
 Calculating N assuming perfect ocean connection.
   * Forced a read of input stream 'ismip-gis' from time: 2094-07-01_00:00:00
   * Forced a read of input stream 'ismip-gis' from time: 2093-07-01_00:00:00
 deltatForcing = 31536000.0000000
...
 Starting timestep number 9
   Setting time step (days) to: 92.2976620370370
 Calculating N assuming perfect ocean connection.
   * Forced a read of input stream 'ismip-gis' from time: 2095-07-01_00:00:00
   * Forced a read of input stream 'ismip-gis' from time: 2094-07-01_00:00:00
 deltatForcing = 31536000.0000000
 

MALI usage example: MALI-Dev#18

[matthewhoffman]

@mgduda , you are absolutely right that with this change, you would no longer have the option to get the time that was read as an argument and instead would have to access the stream cursor to get the new attribute. I could certainly leave the existing functionality in place for backwards compatibility, but it would fail to address the issue that actualWhen is ambiguous if multiple streams were read. In my mind, that potential ambiguity is more dangerous than the loss of convenience associated with the change, but I'm definitely open to other implementations that would preserve backwards compatibility if you have any. One compromise could be to leave the old behavior in place, and leave it to the developer to know when actualWhen is safe to use (or add a warning or error if it isn't).

On a related note, the way I implemented it, one has to explicitly set the saveActualWhen argument to force the time to be saved to the stream manager. It may be more sensible to just have it always occur.

[xylar]

I'm reviewing these changes mostly from a do-no-harm perspective. Since this is not a functionality that we currently use in MPAS-Ocean, nor that we have a short-term need for, I have not tried to understand all the nuances.

I tested the compass ocean pr test suite on Anvil with Intel and Intel-MPI. All tests passed and matched bit-for-bit with a baseline created from a recent E3SM/master (3523b2b).

[matthewhoffman]

MPAS framework team discussed this PR today and decided that this functionality is unlikely to be used by ocean and sea ice, and maintaining identical framework to NCAR/MPAS-Atmosphere is not a requirement at this point, so we will proceed with the version implemented here, pending approval by remaining reviewers. Any of the modifications discussed above could be implemented in the future if needs arise.

[matthewhoffman]

After talking with @trhille , we decided it would be a cleaner implementation to have the new attribute always saved and remove the saveActualWhen argument. I will work on a fix for that today. So reviewers will want to wait until that is complete before looking further at this.

[mark-petersen]

This passes the nightly regression suite with both intel and gnu on grizzly. I'm happy for this to be merged.

[mark-petersen]

Just saw the previous comment by @matthewhoffman. I can review again after PR is updated.

[matthewhoffman]

@trhille , I've pushed a new commit that makes it so the mostRecentAccess time is always recorded. Can you retest this branch for your application, and if you confirm it works properly, tag the reviewers to then proceed with their final testing/review?

[matthewhoffman]

Update: after testing commits to make the actualWhen time always saved, we found that this change was unacceptable because it was not possible to reliably parse existing i/o error messages to identify if xtime was present or not. (The dreaded generic MPAS_IO_ERR_PIO code was returned instead of more specific, useful error codes.) Rather than further modify the i/o framework to deal with that, we decided we would just revert to the version from yesterday that requires the calling routing to request saveActualWhen for calls in which that information is desired. I left the failed attempt in the commit history (now reverted) for posterity. I also updated the subroutine description to explain why saveActualWhen is an optional argument that by default is false.

The code should now be identical to the version that Xylar and Mark tested earlier this week, but Trevor will retest his use case to be sure, and then it's probably best practice to have at least one of Xylar or Mark retest as well.

[matthewhoffman]

@xylar , @mark-petersen , @akturner , I have pushed one new commit to this PR that @trhille found during his final testing. Trevor has successfully tested this final version now, so the PR is ready for final review and then merge.

[mark-petersen]

I recompiled with the head with gnu, and this passes the MPAS-O nightly regression suite again. Thanks.

[xylar]

Thanks @mark-petersen. I don't feel the need to retest, so my approval stands.

[matthewhoffman]

Thanks, Mark & Xylar. @akturner , do you still want to review this before we proceed?

[jonbob]

Passes SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_intel, merged to next

[jonbob]

merged to master. @trhille please feel free to delete the branch from your fork