-
Notifications
You must be signed in to change notification settings - Fork 300
Portable comparisons (for sorting) #1709
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
Changes from all commits
a8315d3
22b6fee
2627d8e
13f7771
909796b
b38cd66
bc96904
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,16 @@ | ||
| * NetCDF OPeNDAP (priority 6) | ||
| * NAME III (priority 5) | ||
| * NetCDF_v4 (priority 5) | ||
| * UM Post Processing file (PP) (priority 5) | ||
| * NetCDF (priority 5) | ||
| * NetCDF 64 bit offset format (priority 5) | ||
| * NetCDF_v4 (priority 5) | ||
| * UM Post Processing file (PP) (priority 5) | ||
| * UM Fieldsfile (FF) post v5.2 (priority 4) | ||
| * UM Fieldsfile (FF) pre v3.1 (priority 3) | ||
| * UM Fieldsfile (FF) ancillary (priority 3) | ||
| * UM Post Processing file (PP) little-endian (priority 3) | ||
| * UM Fieldsfile (FF) converted with ieee to 32 bit (priority 3) | ||
| * UM Fieldsfile (FF) ancillary converted with ieee to 32 bit (priority 3) | ||
| * NIMROD (priority 3) | ||
| * ABF (priority 3) | ||
| * ABL (priority 3) | ||
| * NIMROD (priority 3) | ||
| * UM Fieldsfile (FF) ancillary (priority 3) | ||
| * UM Fieldsfile (FF) ancillary converted with ieee to 32 bit (priority 3) | ||
| * UM Fieldsfile (FF) converted with ieee to 32 bit (priority 3) | ||
| * UM Fieldsfile (FF) pre v3.1 (priority 3) | ||
| * UM Post Processing file (PP) little-endian (priority 3) | ||
| * GRIB (priority 1) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,7 +96,8 @@ def assert_bounded_message(self, **kwargs): | |
| 'edition': 1, '_forecastTime': 15, | ||
| '_forecastTimeUnit': 'hours', | ||
| 'phenomenon_bounds': lambda u: (80, 120), | ||
| '_phenomenonDateTime': -1} | ||
| '_phenomenonDateTime': -1, | ||
| 'table2Version': 9999} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change supposed to be here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, somewhere in the GRIB loader is
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK - thanks for clarifying. 👍 |
||
| attributes.update(kwargs) | ||
| message = mock.Mock(**attributes) | ||
| self._test_for_coord(message, convert, self.is_forecast_period, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really want this list changing order ... that indicates a potential change in behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order comes from the
FormatSpecificationcomparitor:Eww 😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the simplest solution is to tweak the definitions in
lib/iris/fileformats/__init__.pyto use unique numbers that preserve the current order?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed (perhaps incorrectly) that as long as they are at the same priority, it should not matter which one is ordered first. I don't think the tests have complained so far...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format specifications with the same priority should have mutually exclusive file matching criteria, in which case we would be able to get away with it. I'll check...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly they are mutually exclusive, but sadly not in the case of the ABF/L formats vs. the other "priority 3" formats. For example, a FieldsFile named "foo.abf" would load OK using the old order, but would try to load as an ABF file using the new order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB. We really shouldn't be relying on consistent hash results anyway - e.g. http://bugs.python.org/issue13703.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the minimal impact I suggest we just accept the change as it makes the order more predictable and controllable.