Remove bugs when reading 360 day calendar LBCs #3743
Conversation
|
ok - so half of the Travis CI tests failed, I'd like some help in interpreting those failures too, as on a quick scan, I can't see what the problem is |
|
I now see what the error is |
|
|
ok, so all that is now failing is black. I am confused - I thought that Iris automatically applied black when I commit? How can I fix this? |
|
so now I have all the tests passing the next question is what additional tests should I be adding to the code? At the moment, the tests for LBCs are all based on mock. Should we add some real data as well? Also I would like to see tests for: 360 day calendar and variable resolution LBCs in addition to the fairly standard version we have here. |
|
Hi @nhsavage, thanks for your work on this; it's good to see the open source model working as intended. Since you are within the Met Office, it would be good to discuss your outstanding questions in detail at our next surgery on Thursday 2nd July. Does that work for you? We wouldn't plan to merge this between now and then anyway (due to some other significant changes currently in the works). |
|
Sorry Martin,
that doesn’t work for me because the next one is when I am taking annual leave (I was meant to be walking in the Highlands, instead I’ll be lucky to make it to Dartmoor…)
Nick
From: Martin Yeo <notifications@github.com>
Sent: 19 June 2020 13:38
To: SciTools/iris <iris@noreply.github.com>
Cc: Savage, Nicholas <nicholas.savage@metoffice.gov.uk>; Mention <mention@noreply.github.com>
Subject: Re: [SciTools/iris] Remove bugs when reading 360 day calendar LBCs (#3743)
Hi @nhsavage<https://github.com/nhsavage>, thanks for your work on this; it's good to see the open source model working as intended.
Since you are within the Met Office, it would be good to discuss your outstanding questions in detail at our next surgery on Thursday 2nd July. Does that work for you? We wouldn't plan to merge this between now and then anyway (due to some other significant changes currently in the works).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#3743 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABPAHXQSH5Z7JPX35NXFEZDRXNLZ7ANCNFSM4OA32OLQ>.
|
There was a problem hiding this comment.
Hi @nhsavage. Following offline discussion, we have agreed that this PR needs:
- A minor change to
lbtim_default - A few more unit tests, with mock headers representing a wider range of examples of LBC files. Use your judgement on how many is appropriate 🙂
As a follow-on I have also raised #3748 to further enhance LBC test coverage in future.
Once the requested changes have been made, I'm keen to get a review from another user experienced with LBC's. We can see if we can find someone with a GitHub account, or get them to open one!
| @@ -577,12 +577,8 @@ def _adjust_field_for_lbc(self, field): | |||
| # Set LBTIM to indicate the specific time encoding for LBCs, | |||
| # i.e. t1=forecast, t2=reference | |||
| lbtim_default = 11 | |||
There was a problem hiding this comment.
| lbtim_default = 11 | |
| lbtim_default = pp.SplittableInt(11) |
Update CI environment lockfiles
|
@SciTools/peloton any chance of progressing this @nhsavage ? |
Probably not realistically. At the time of the last activity, I hadn't use mocking in tests before and so was stuck. I don't have any time specifically to work on this (it is useful but not essential to our work to do this and we have managed for 3 years without it. Is there any demand for this feature/fix from anyone except me? If not I suggest we close this |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3743 +/- ##
=======================================
Coverage ? 89.27%
=======================================
Files ? 88
Lines ? 22257
Branches ? 4867
=======================================
Hits ? 19870
Misses ? 1641
Partials ? 746 ☔ View full report in Codecov by Sentry. |
|
|
For #3741. This code now is able to read the sample LBC files with a 360 day calendar. I am now using this pull request to run the standard tests and seek further advice on adding extra tests (there don't seem to be any LBC files at the moment in the Iris data collection) and on whether it is desirable to check the values of LBTIM in some way.