Translate text PP save rules into Python #2795
Conversation
DPeterK
force-pushed
the
pp_save_rules_py
branch
from
0af9d9b to
314e0e3
Compare
| self.patcher.start() | ||
|
|
||
| def tearDown(self): | ||
| self.patcher.stop() |
There was a problem hiding this comment.
@dkillick Might want to take a peek at iris.tests.patch ...
| 'default': np.dtype('>f4'), | ||
| } | ||
|
|
||
| # LBPROC codes and their English equivalents |
There was a problem hiding this comment.
I think moving these to iris.fileformats.rules is a mistake, as it the information is specific to PP (and FF), whereas iris.fileformats.rules "ought" to be the home for fileformat-independent rules processing.
They aren't specially tied to implementation of text rules anyway.
In any case, "LBPROC_PAIRS" and "lbproc_map" are perfectly reasonable public elements of the API, so you can't just move them anyway.
In an ideal world I think these would be in "iris.fileformats.um_cf_map", and also "lbroc_map" should be capitalised, but it's a bit late for that...
There was a problem hiding this comment.
(from discussion with @dkillick)
The move was introduced because of a circularity problem when iris.fileformats.pp imports iris.fileformats.pp_save_rules, which imports iris.fileformats.pp ...
By putting 'lbproc_map' into rules, then pp_save_rules no longer needed to import pp.
Alternative suggestion: Put LBPROC_PAIRS and lbproc_map inside pp_save_rules itself.
Then to avoid breaking existing API, publish them also in 'pp' by importing them from 'pp_save_rules'.
For better measure, we can fix naming + make them private in pp_save_rules.
Ideally we would "deprecate" this, but I also can't see how?
So, perhaps this really is a suitable case for a major-version breaking change ??
There was a problem hiding this comment.
More trouble...
I missed that "lbproc_map" is also used in "pp_rules"
( i.e. "pp_load_rules", perhaps it should now be called that ? )
You can import it there from "pp_save_rules", but that is obviously a rather peculiar dependency ordering.
You cannot import it from 'pp', if 'pp' is to import 'pp_rules' (same circularity problem as before)
In my view, this info really belongs with the other similar stuff in 'um_cf_map'.
To do this properly, we should really put this info into metarelate, and modifytools/gen_translations.py to support it, but I really don't think we have time for that.
In which case, we should just host this translation info in a separate module.
See : DPeterK#12 for worked suggestion ...
DPeterK
force-pushed
the
pp_save_rules_py
branch
from
6f8c233 to
4bbaae6
Compare
|
On line 64 of pp.py, _save_rules is set to none. The functionality of _save_rules was removed in a previous pull request and with the changes made in this PR it is now obsolete. It would probably be best to remove lines 64 (and maybe 63) now. Otherwise I see no issues with this PR and would approve it. |
* Move lbproc_map to avoid circular import * Reorganisation of pp lbproc_pairs feature.
|
Finally!! Pats on the back all round for that one. |
6 participants
All the PP save rules were supplied by a text file
lib/iris/etc/pp_save_rules.txtbut the text rule processing classes inlib/iris/fileformats/rules.pyare deprecated. This PR, therefore, takes the text rules and translates them into a new Python modulelib/iris/fileformats/pp_save_rules.pyand makes the necessary changes to PP save so that the new save rules are run.The old text rules have been updated and a very few test changes have been made – but, of course, this job will have been done correctly if the tests all pass as-is with the exception of the odd API change.
This unblocks #2782.