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

Nimrod file format #3647

Merged
merged 49 commits into from
Jun 4, 2020
Merged

Nimrod file format #3647

merged 49 commits into from
Jun 4, 2020

Conversation

MoseleyS
Copy link
Contributor

@MoseleyS MoseleyS commented Jan 27, 2020

Enriches the meta-data inherited from Nimrod-format data and the data-types associated with coords.
Requires new iris-test-data from SciTools/iris-test-data#55

@SciTools-assistant SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Jan 27, 2020
…tions.

Adds forecast_period coord.
Adds more Nimrod data to unit-tests to cover known operational data.
@SciTools-assistant SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Jan 31, 2020
- Improves handling of missing data values.
- Improves handling for strings with null characters present.
- Replaces instances of "unknown" units with Unit("1").
Ran black on the code base and committed the results.
I think I've blacked-out.
MoseleyS added 5 commits May 22, 2020 14:48
- Distinguishes between precipitation rate and accumulation
- Raises a warning if the coordinate reference system is incomplete
- Expands doc-strings for coord system functions
- Rearranges warning message to make it easier to read.
@pp-mo
Copy link
Member

pp-mo commented May 22, 2020

Hi @MoseleyS .
Warning : tests have not been run on the latest commit..
Travis testing is not triggering reliably + I'm not sure why.
I see you had this problem yesterday, whereas I was getting tests earlier on today but it just stopped for me.

MoseleyS and others added 5 commits May 22, 2020 15:25
Copy link

@fionaRust fionaRust left a comment

Choose a reason for hiding this comment

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

A few more small comments and at least one thing I still don't understand fully.

I think we can also cut back on the number of cml files and/or the number of cubes in each one. A lot of them are testing data with the same kind of data, just with different names. We may want to think about how useful this is. At the moment it is impossible to check it all carefully because there is so much of it.

)
elif field.horizontal_grid_type == 1:
coord_sys = iris.coord_systems.GeogCS(**ellipsoid)
else:
raise TranslationError(

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't clearly see the context, but I think it is because this Translation Error is triggered by another function and doesn't need to be in the code-base twice.

Choose a reason for hiding this comment

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

That's fine

add_attr(key)

# Remove member number from cube_source
match = re.match(

Choose a reason for hiding this comment

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

Might need to explain this to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment expanded to explain what and why is going on here.

@@ -909,6 +926,7 @@ def run(field):
experiment(cube, field)

# horizontal grid
origin_corner(cube, field)
horizontal_grid(cube, field)

Choose a reason for hiding this comment

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

Should put in horizontal_grid docstring that it should come after origin_corner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

</coord>
<coord datadims="[0]">
<auxCoord bounds="[[0.0, 304.8],
[0.0, 30000.0]]" id="a2cbc1ea" points="[0.0, 0.0]" shape="(2,)" standard_name="height" units="Unit('m')" value_type="float32">

Choose a reason for hiding this comment

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

I'm not sure I understand a lot of the probability metadata in this file. Can we go through it together?

@@ -0,0 +1,43 @@
<?xml version="1.0" ?>

Choose a reason for hiding this comment

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

Do we need this test as well as the lib/iris/tests/results/nimrod/u1096_ng_bmr04_precip_2km.cml test

Choose a reason for hiding this comment

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

Also would it be possible to rename the test and result files to make them a bit easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file names come from the operational suite file names. Only the initial date string has been stripped off. I did this so I could be confident I'd tested all the use-cases I knew of.

<dimCoord id="201cc2ba" points="[1580180400]" shape="(1,)" standard_name="forecast_reference_time" units="Unit('seconds since 1970-01-01 00:00:00', calendar='gregorian')" value_type="int64"/>
</coord>
<coord datadims="[0]">
<dimCoord id="a2cbc1ea" points="[30.0, 60.0, 90.0, 120.0, 150.0, 180.0, 210.0,

Choose a reason for hiding this comment

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

Is it expected that this will load two separate cubes? From a first look they look like they should be merge-able?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is correct. The difference is <attribute name="num_model_levels"/> where value="57" or value="26". These are two different level-sets that should remain separate.

@pp-mo
Copy link
Member

pp-mo commented May 27, 2020

Hi @MoseleyS what's the state of this now, are there still points above you want to address ?

I'm currently proposing an addition to the load functions if you wanted to use that, as you suggested above

@MoseleyS
Copy link
Contributor Author

Hi @MoseleyS what's the state of this now, are there still points above you want to address ?

I'm currently proposing an addition to the load functions if you wanted to use that, as you suggested above

Hi Patrick. Yes, there are still a few points to address here. I'm hoping to look at them today (28th). The loader kwargs looks like it will allow us to optionally disable some things - I'll have a look more closely at that.

MoseleyS added 4 commits May 28, 2020 10:09
…mrod_file_format

* 'nimrod_file_format' of github.com:MoseleyS/iris:
  Removed unnecessary dictionaries from name function
  Adding unit tests for unit function
@MoseleyS
Copy link
Contributor Author

Hi @pp-mo
I've added a kwarg to allow the user to suppress the handling of known meta-data issues. I've also added a test for this, but of course the test fails because this PR doesn't include the one that introduces kwargs handling. Not quite suite how to proceed with this now.

):
iris.load(
tests.get_data_path(("NIMROD", "uk2km", "cutouts", datafile,)),
handle_metadata_errors=False,
Copy link
Member

@pp-mo pp-mo May 28, 2020

Choose a reason for hiding this comment

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

For the load api extension proposed in #3720, this isn't the correct syntax.
Instead, it will need to set the loader_kwargs keyword,
e.g. in this case ..., loader_kwargs={'handle_metadata_errors':False})

Copy link
Member

@pp-mo pp-mo May 28, 2020

Choose a reason for hiding this comment

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

Of course, #3720 isn't agreed yet,either ...

@pp-mo
Copy link
Member

pp-mo commented May 28, 2020

of course the test fails because this PR doesn't include the one that introduces kwargs handling

Well, you can either wait, or you could re-organise the test to use the format-specific interface instead.
I.E. replace the existing iris.load call with iris.fileformats.nimrod.load_cubes.
It's not quite the same, as there is no merge step, but in the context of this test I think it should work fine.

MoseleyS added 2 commits May 28, 2020 14:58
- handle_metadata_errors is no longer a kwarg via iris.load_cube as the functionality is not in master yet and wasn't coded correctly here anyway
- Removes "_above_threshold" from cloud field id 161
- Removing of zero-length bounds has been added but disabled until the merge_cube method stops stripping bounds where one cube has bounds=None.
Copy link

@fionaRust fionaRust left a comment

Choose a reason for hiding this comment

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

I'm going to approve this now. You've addressed my latest set of comments and I've checked that the handle_metadata_errors functionality will turn off the right bits of the code if it is enabled. I've not checked there are sufficient tests for this functionality

@pp-mo pp-mo merged commit a07800c into SciTools:master Jun 4, 2020
@pp-mo
Copy link
Member

pp-mo commented Jun 4, 2020

Many thanks for all your patience @MoseleyS !

tkknight pushed a commit to tkknight/iris that referenced this pull request Jun 29, 2020
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.

6 participants