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

Adds a CF-compliant time dimension for Omega IO #169

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

philipwjones
Copy link

@philipwjones philipwjones commented Nov 22, 2024

Adds a CF-compliant unlimited time dimension

  - adds support for an unlimited time dimension
  - adds the CF-compliant time field with appropriate units
  - adds additional Field properties to support time-dependent fields
  - extends the support for non-distributed field IO into IOStream
  - updates documentation

This still only supports one time slice per file. A subsequent modification will add support for multiple slices in a file.

No additional unit tests were added, but I have manually confirmed the fields and dimensions appear correctly in the output files. Additional tests will be added as part of the upcoming multi-slice capability.

Checklist

  • Documentation:
    • Developer's Guide has been updated
    • Documentation has been built locally and changes look as expected
  • Testing
    • A comment in the PR documents testing used to verify the changes including any tests that are added/modified/impacted.
    • Unit tests have passed. Please provide a relevant CDash build entry for verification.

  - adds support for an unlimited time dimension
  - adds the CF-compliant time field with appropriate units
  - adds additional Field properties to support time-dependent fields
  - extends the support for non-distributed field IO into IOStream
  - updates documentation

This still only supports one time slice per file. A subsequent modification
will add support for multiple slices in a file.
Copy link

@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.

@philipwjones, this looks really excellent! I successfully ran CTests on Chrysalis with Intel and OpenMPI but I'm still working on some meaningful tests in Polaris.

In the meantime, I have some comments that I think will be easy to address.

components/omega/src/infra/Field.h Outdated Show resolved Hide resolved
components/omega/doc/devGuide/Field.md Show resolved Hide resolved
components/omega/src/infra/Field.cpp Outdated Show resolved Hide resolved
components/omega/src/infra/Field.cpp Outdated Show resolved Hide resolved
components/omega/doc/devGuide/Field.md Outdated Show resolved Hide resolved
@xylar
Copy link

xylar commented Nov 28, 2024

I'm sorry for the delay in reviewing this. I wanted to test in Polaris, but needed to get E3SM-Project/polaris#231 updated based on recent changed to Omega's develop branch before I could test this out. That is finally done and I'm testing this.

@xylar
Copy link

xylar commented Nov 28, 2024

Beautiful!!

I'm seeing:

$ ncdump -v time output.nc 
netcdf output {
dimensions:
	MaxCellsOnEdge = 2 ;
	MaxEdges = 6 ;
	NCells = 2500 ;
	NEdges = 7500 ;
	NTracers = 5 ;
	NVertLevels = 1 ;
	NVertices = 5000 ;
	VertexDegree = 3 ;
	time = UNLIMITED ; // (1 currently)
variables:
...
	double time(time) ;
		time:Description = "time" ;
		time:FillValue = 0. ;
		time:Name = "time" ;
		time:StdName = "time" ;
		time:Units = "seconds since 0001-01-01 00:00:00" ;
		time:ValidMax = 1.e+20 ;
		time:ValidMin = 0. ;
		time:_FillValue = 0. ;
		time:calendar = "noleap" ;
		time:long_name = "time" ;
		time:name = "time" ;
		time:standard_name = "time" ;
		time:units = "seconds since 0001-01-01 00:00:00" ;
		time:valid_max = 1.e+20 ;
		time:valid_min = 0. ;

// global attributes:
		:SimulationTime = "0001-01-01_10:00:00" ;
data:

 time = 36000 ;
}

This is correct: we wanted to write out data 10 hours = 36000 seconds into the simulation.

@philipwjones, my only suggestion would be that 0. is not a safe FillValue and _FillValue. This should be something like -9.99e+30 used for LayerThickness.

I the meantime, I'll be working on making the manufactured solution analysis step in Polaris work with this update.

std::string CalName = CalendarCFName[CalKind];
std::vector<std::string> DimNames; // empty dim names vector
std::shared_ptr<Field> TimeField =
create("time", "time", UnitString, "time", 0.0, 1.e20, 0.0, 0, DimNames);
Copy link

Choose a reason for hiding this comment

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

Suggested change
create("time", "time", UnitString, "time", 0.0, 1.e20, 0.0, 0, DimNames);
create("time", "time", UnitString, "time", 0.0, 1.e20, -9.9e30, 0, DimNames);

Copy link
Author

Choose a reason for hiding this comment

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

Thanks - done.

Copy link

@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.

Thanks @philipwjones, this looks great!

@brian-oneill
Copy link

Seems good to me. Successfully passed the unit tests on Chrysalis and Perlmutter CPU & GPU. If you all are happy, I approve.

@philipwjones philipwjones merged commit da87361 into E3SM-Project:develop Dec 3, 2024
2 checks passed
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