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

Assume mesh variables only in mesh file #97

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

cbegeman
Copy link
Collaborator

For OMEGA, mesh variables will be located in a separate netcdf file from the output. This PR always loads mesh variables from the mesh netcdf (usually mesh.nc or culled_mesh.nc). Some of my changes from the init netcdf to the mesh netcdf file are unnecessary but I think it improves code clarity.

Checklist

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

@cbegeman cbegeman requested a review from xylar July 20, 2023 14:29
@cbegeman cbegeman self-assigned this Jul 20, 2023
@cbegeman cbegeman added clean-up ocean Related to ocean tests or analysis labels Jul 20, 2023
@cbegeman
Copy link
Collaborator Author

Testing

This PR passes PR and cosine_bell suites on chrys with intel-mpi

@xylar
Copy link
Collaborator

xylar commented Jul 20, 2023

I'm curious about the various ds.close() calls. I assume that was just to tidy things up by being explicit about the files being closed? The files should always be closed automatically when the ds object gets cleaned up at the end of the function.

@xylar
Copy link
Collaborator

xylar commented Jul 20, 2023

The slightly preferred way to handle scope for an xarray dataset that isn't needed for the full scope of the function or method is something like:

with xr.open_dataset('mesh.nc') as ds_mesh:
    xCell = ds_mesh.xCell.values
    ...

In this case, ds_mesh.close() is automatically called at the end of the with statement and the scope of ds_mesh is clear. It is important not to refer to ds_mesh or any of its data arrays outside of that scope.

I don't think we likely want to do that retroactively because it changes a lot of whitespace but I would consider it if it's important to you to have an indication of the scope of ds, ds_mesh, etc. I have a slight preference to not having explicit ds.close() calls as that isn't a common practice but could accept them, too.

@cbegeman
Copy link
Collaborator Author

I'm curious about the various ds.close() calls. I assume that was just to tidy things up by being explicit about the files being closed? The files should always be closed automatically when the ds object gets cleaned up at the end of the function.

I'm happy to remove them if they're not necessary

@cbegeman cbegeman force-pushed the always-use-mesh-vars branch from c00621f to 7dbf393 Compare July 21, 2023 01:26
@cbegeman
Copy link
Collaborator Author

@xylar ds.close gone now. Ready for re-review.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Wonderful! Thanks for making these changes and testing them thoroughly. This is a good idea for MPAS-Ocean and obviously a necessary change for OMEGA.

@xylar
Copy link
Collaborator

xylar commented Jul 21, 2023

Thanks for engaging with my nit-picking on ds.close().

@cbegeman cbegeman merged commit e45ad63 into E3SM-Project:main Jul 21, 2023
@xylar xylar deleted the always-use-mesh-vars branch August 6, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants