Skip to content

Conversation

@wjbenfold
Copy link
Contributor

No description provided.

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Had a quick look at the proposed file, but I think it could do with some tidying ...

$ cfchecker test_data/NetCDF/mercator/false_east_north_merc.nc
Running Met Office fork of cf-checker.
CHECKING NetCDF FILE: test_data/NetCDF/mercator/false_east_north_merc.nc
=====================
Parsing standard names file /home/h03/cdds/software/cfchecker_rhel7/etc/cf-standard-name-table.xml
Using CF Checker Version 3.1.0
Checking against CF Version CF-1.7
Using Standard Name Table Version 67 (2019-06-18T13:25:38Z)
Using Area Type Table Version 9 (07 August 2018)
Using Standardized Region Name Table Version 4 (18 December 2018)

WARN: (2.6.1): No 'Conventions' attribute present
ERROR: (5): coordinates attribute referencing non-existent variable
ERROR: (5): coordinates attribute referencing non-existent variable
ERROR: (5.6): grid_mapping attribute referencing non-existent variable crs
ERROR: (7.1): bounds attribute referencing non-existent variable time_bnds

 . . .
Checking variable: psl ...
Checking variable: time ...
Checking variable: x ...
Checking variable: y ...

ERRORS detected: 4
WARNINGS given: 1
INFORMATION messages: 0
$

( BTW I'm really not sure why the MO has our own fork of the cfchecker, but I think it has no bearing on this case. )

If you checkout the ncdump, you can see some 'broken' links
BUT MOST NOTABLY there is no grid-mapping variable.

 $ ncdump -h test_data/NetCDF/mercator/false_east_north_merc.nc 
netcdf false_east_north_merc {
dimensions:
	time = UNLIMITED ; // (1 currently)
	y = 10 ;
	x = 10 ;
variables:
	float psl(time, y, x) ;
		psl:_FillValue = 1.e+20f ;
		psl:cell_methods = "time: mean" ;
		psl:long_name = "Sea Level Pressure" ;
		psl:standard_name = "air_pressure_at_sea_level" ;
		psl:units = "Pa" ;
		psl:coordinates = "lat lon" ;
		psl:grid_mapping = "crs" ;
	double time(time) ;
		time:long_name = "time" ;
		time:standard_name = "time" ;
		time:units = "days since 1949-12-01 00:00:00" ;
		time:calendar = "proleptic_gregorian" ;
		time:bounds = "time_bnds" ;
	double x(x) ;
		x:long_name = "x-coordinate in Cartesian system" ;
		x:standard_name = "projection_x_coordinate" ;
		x:units = "m" ;
		x:axis = "X" ;
	double y(y) ;
		y:long_name = "y-coordinate in Cartesian system" ;
		y:standard_name = "projection_y_coordinate" ;
		y:units = "m" ;
		y:axis = "Y" ;
}

Something must have gone wrong ?
We can contact offline to fix this @wjbenfold ?

@wjbenfold
Copy link
Contributor Author

I got this file from @nhsavage, who had stripped down an existing file and in doing so removed some pieces that were inconsistent with each other (on the basis that missing data is better than bad data). Given that using this file my changes to Iris do do the right thing, is it then ok to generate the test data that goes in this repo using Iris? Or should we look for another file that is internally consistent and complete?

@nhsavage
Copy link

I have clearly been too enthusiastic in my stipping out of redundant stuff. Let me have another go...

@pp-mo
Copy link
Member

pp-mo commented Feb 14, 2022

should we look for another file that is internally consistent and complete?

I think it's fine to just visually inspect the ncdump, and so confirm that the data has the properties we wanted.
If the file is generated with Iris, I wouldn't say that simply rules it out !

@nhsavage
Copy link

psl_mercator_example5.nc passes all of the cfchecker (in same place as before)

@wjbenfold wjbenfold requested a review from pp-mo February 15, 2022 16:46
@pp-mo
Copy link
Member

pp-mo commented Feb 15, 2022

Ok, this now looks a lot more sensible! 😁
ncdump -h :

netcdf false_east_north_merc {
dimensions:
	time = UNLIMITED ; // (1 currently)
	y = 10 ;
	x = 10 ;
	bnds = 2 ;
variables:
	char crs ;
		crs:proj4_params = "+proj=merc +lat_ts=-2.00 +lon_0=12.00 +x_0=-12500. +y_0=-12500. +ellps=sphere +a=6371229. +b=6371229. +units=m +no_defs" ;
		crs:semi_major_axis = 6371229. ;
		crs:inverse_flattening = 0. ;
		crs:false_easting = -12500. ;
		crs:false_northing = -12500. ;
		crs:grid_mapping_name = "mercator" ;
		crs:standard_parallel = -2. ;
		crs:latitude_of_projection_origin = -2. ;
		crs:longitude_of_projection_origin = 12. ;
	float psl(time, y, x) ;
		psl:_FillValue = 1.e+20f ;
		psl:cell_methods = "time: mean" ;
		psl:long_name = "Sea Level Pressure" ;
		psl:standard_name = "air_pressure_at_sea_level" ;
		psl:units = "Pa" ;
		psl:grid_mapping = "crs" ;
	double time(time) ;
		time:long_name = "time" ;
		time:standard_name = "time" ;
		time:units = "days since 1949-12-01 00:00:00" ;
		time:calendar = "proleptic_gregorian" ;
		time:bounds = "time_bnds" ;
	double time_bnds(time, bnds) ;
	double x(x) ;
		x:long_name = "x-coordinate in Cartesian system" ;
		x:standard_name = "projection_x_coordinate" ;
		x:units = "m" ;
		x:axis = "X" ;
	double y(y) ;
		y:long_name = "y-coordinate in Cartesian system" ;
		y:standard_name = "projection_y_coordinate" ;
		y:units = "m" ;
		y:axis = "Y" ;

// global attributes:
		:Conventions = "CF-1.7" ;
}

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

I think the file makes sense now.
But, don't you need to bump the release version ?
(unless there is more to go in before next release)

@wjbenfold
Copy link
Contributor Author

I think the file makes sense now. But, don't you need to bump the release version ? (unless there is more to go in before next release)

I think the github action bumps the release automatically? https://github.com/SciTools/iris-test-data/blob/v2.6/.github/workflows/release.yml

@pp-mo
Copy link
Member

pp-mo commented Feb 15, 2022

github action bumps the release automatically

Sorry, my understanding was out of date!

@pp-mo pp-mo merged commit 0c891b2 into SciTools:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants