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

Ugrid cf load #3684

Merged
merged 9 commits into from
Mar 20, 2020
Merged

Ugrid cf load #3684

merged 9 commits into from
Mar 20, 2020

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Mar 13, 2020

DO NOT MERGE
Should re-target this at branch "ng-vat"

Did try that, but at present 'ng-vat' is stale, wanting a mergeback from master. See : #3685
Do not merge until that is resolved : this can then be rebased onto + re-targetted at ng-vat.

#3685 now merged, this PR should be re-pointed at ng-vat.

@pp-mo pp-mo changed the base branch from master to ng-vat March 15, 2020 15:29
@pp-mo pp-mo changed the base branch from ng-vat to master March 15, 2020 15:40
@pp-mo pp-mo requested a review from stephenworsley March 15, 2020 16:14
@pp-mo
Copy link
Member Author

pp-mo commented Mar 18, 2020

Thanks for some eyes on this @lbdreyer @alastair-gemmell

I'm wondering if it is worth getting the test-data up somewhere online?
Then we could merge this + have the test functioning.

An alternative might be to have more focussed unit-tests, which could work from a synthetic file (somehow?). But I think that is probably overkill + wasted time at this stage, No ?

@lbdreyer
Copy link
Member

Thanks for some eyes on this @lbdreyer @alastair-gemmell

I'm wondering if it is worth getting the test-data up somewhere online?
Then we could merge this + have the test functioning.

An alternative might be to have more focussed unit-tests, which could work from a synthetic file (somehow?). But I think that is probably overkill + wasted time at this stage, No ?

My preference would be towards using real data (not synthetic data) so we just need to get approval for a test file we can make public.
Are you happy to add the new file to iris-test-data? Or were you thinking of putting it somewhere else

@pp-mo
Copy link
Member Author

pp-mo commented Mar 18, 2020

Well, here's a thing.
When I went to put a testfile in iris-test-data, there was already much older tiny XIOS file in there.
So I've just quickly rewritten as a real test that checks various aspects of the loaded result.
It seems to work to me, though clearly it is in a rather older format.

How do you feel about that, @lbdreyer @alastair-gemmell ?

@alastair-gemmell
Copy link
Contributor

Well, here's a thing.
When I went to put a testfile in iris-test-data, there was already much older tiny XIOS file in there.
So I've just quickly rewritten as a real test that checks various aspects of the loaded result.
It seems to work to me, though clearly it is in a rather older format.

How do you feel about that, @lbdreyer @alastair-gemmell ?

I suppose it depends on how different the older format is to the newer formats, balanced against the time and effort required to get a more up-to-date test file to the test repo?

@pp-mo
Copy link
Member Author

pp-mo commented Mar 18, 2020

how different the older format is

Well it is a valid UGRID format, and I think that maybe good enough for now.
My only slight disappointment is that this file has only one grid in it, so we are not testing all the functionality that we might be. But as I've checked that aspect thoroughly, and it does work for now, I'm not that bothered.

In the meantime, there are a bunch of consequential errors in existing tests.
I'll fix those + see where it takes me.

@pp-mo
Copy link
Member Author

pp-mo commented Mar 18, 2020

a bunch of consequential errors in existing tests.

I fixed the few tests that create a CFReader directly, since its API has changed.

The remaining problems were caused by existing regridding tests that use the test file theta_nodal_xios.nc.
Since that is now interpreted as ugrid on loading, it messes these ones up.

SciTools/iris-test-data#56 introduces a hacked version of that file,
where I have renamed the defining "cf_role" property of the mesh node, so it won't be recognised as ugrid format.
The latest commit mods the tests to use that version, and they should pass once we have an updated iris-test-data to point it at ...

@bjlittle bjlittle added the PoC label Mar 19, 2020
@bjlittle bjlittle modified the milestone: PoC Mar 19, 2020
@pp-mo pp-mo changed the base branch from master to ng-vat March 19, 2020 11:38
@pp-mo
Copy link
Member Author

pp-mo commented Mar 19, 2020

Hang on a tick please.
Now rebasing onto ng-vat ...

@pp-mo
Copy link
Member Author

pp-mo commented Mar 19, 2020

rebasing onto ng-vat ...

Done.
Enjoy ! 🍨 🎈

@pp-mo
Copy link
Member Author

pp-mo commented Mar 19, 2020

Small changes.
I decided the 'private' nature of the module name was an unnecessary mistake.
Should now test out OK.

pp-mo and others added 2 commits March 20, 2020 11:52
Co-Authored-By: lbdreyer <lbdreyer@users.noreply.github.com>
@pp-mo
Copy link
Member Author

pp-mo commented Mar 20, 2020

'boundary_node_connectivity' as a link property.

Yet another small tweak.
I reckon that this doesn't occur anywhere in the test files, though.

@trexfeathers
Copy link
Contributor

LGTM! 🎉

@trexfeathers trexfeathers merged commit fbdd5ec into SciTools:ng-vat Mar 20, 2020
@pp-mo pp-mo deleted the ugrid_cf_load branch March 18, 2022 14:53
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.

5 participants