Skip to content

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Apr 3, 2025

This alternative approach makes sure the contents are unique (and sorted) in the xtime variable.

To reduce redundancy between the new approach and the old ncrcat approach (retained for comparison and in case the xarray approach is not performant), a new helper function for determining if we want to append to the existing datasets added.

Checklist

  • Testing comment in the PR documents testing used to verify the changes

…ries

This alternative approach makes sure the constents are unique (and
sorted) in the xtime variable.

To reduce redundancy between the new approach and the old ncrcat
approach (retained for comparison and in case the xarray approach
is not performant), a new helper function for determining if we
want to append to the existing datasets added.
@xylar xylar added the bug label Apr 3, 2025
@xylar xylar requested review from cbegeman and darincomeau April 3, 2025 15:04
@xylar xylar self-assigned this Apr 3, 2025
@xylar
Copy link
Collaborator Author

xylar commented Apr 3, 2025

Testing

I ran just the conservation analysis on 20250327.v3.SORRME3r3.piControl.alfred2.chrysalis and it looks reasonable:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/analysis_testing/chrysalis/fix-conservation-redundant-time/20250327.v3.SORRME3r3.piControl.alfred2.chrysalis/

I am also running the test suite and the conservation analysis looks the same as before in tests that have completed so far there:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.xylar/analysis_testing/chrysalis/fix-conservation-redundant-time/

As an example, here is a plot of salt conservation prior to this PR:
image
and with this PR:
image

@xylar
Copy link
Collaborator Author

xylar commented Apr 3, 2025

@cbegeman and @darincomeau, if you could have a look, that would be great. If we're good with these fixes, I can merge this and update the analysis environment for BlueTip to include this fix.

Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

Approving on the basis of code inspection and @xylar's testing. Thanks for fixing this!

@xylar xylar changed the title Add an alternative xarray approach to computing convservation time series Alternative xarray approach to computing convservation time series Apr 3, 2025
@xylar
Copy link
Collaborator Author

xylar commented Apr 3, 2025

Thanks @cbegeman!

@darincomeau
Copy link
Contributor

Thanks @xylar for the quick fix! You're testing looks good to me. I can approve now (it'll be easier to use this in zppy when it's in the new bluetip environment), or I can run the full analysis from here on that alfred2 run.

@xylar
Copy link
Collaborator Author

xylar commented Apr 3, 2025

I think you can just approve. It's not too hard for me to redo thing if something goes wrong later with the updated BlueTip environment.

Copy link
Contributor

@darincomeau darincomeau left a comment

Choose a reason for hiding this comment

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

Approved based on developer testing. Thanks again Xylar!

@xylar xylar merged commit ed6913d into MPAS-Dev:develop Apr 4, 2025
5 checks passed
@xylar xylar deleted the fix-conservation-redundant-time branch April 4, 2025 00:15
@xylar
Copy link
Collaborator Author

xylar commented Apr 4, 2025

Thanks @darincomeau. I'll try to put a new environment in place soon.

@xylar xylar mentioned this pull request Apr 13, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants