Missed call to __init__ of the super class - #3181
Conversation
matrss
left a comment
matrss
left a comment
There was a problem hiding this comment.
I do not feel too confident reviewing this, as I have no experience with what this part of MSS is supposed to do and why it does what it does. Nevertheless, some thoughts.
| return lat_data, lon_data, lat_order | ||
|
|
||
|
|
||
| def _identify_aggregation_dim(dataset, exclude): |
There was a problem hiding this comment.
Why is this function necessary? Are there real word cases where selecting the unlimited dimension (i.e. aggdim=None in the super().__init__ call) is not enough?
| # advance, as the master is not open yet at this point. | ||
| with netCDF4.Dataset(master) as cdf_master: | ||
| aggdim = _identify_aggregation_dim(cdf_master, exclude) | ||
| super().__init__([master], exclude=exclude, aggdim=aggdim) |
There was a problem hiding this comment.
The constructor of MFDataset expects all files that comprise the MFDataset as its first argument, not just the first/master file. Now that I've read the comment above, I am of the opinion that subclassing from MFDataset is just wrong if this class is not supposed to concatenate the files along a dimension.
| if part.file_format == "NETCDF4": | ||
| raise ValueError("MFNetCDF4 only works with NETCDF3_CLASSIC, " | ||
| "NETCDF3_64BIT and NETCDF4_CLASSIC " | ||
| "formatted files, not NETCDF4") |
There was a problem hiding this comment.
It does not seem like a good thing to not support the latest version of netCDF.
There was a problem hiding this comment.
Disagree. NetCDF4 introduces a lot of complicated features. We describe a pretty reasonable format for MSS supported files, which does not use any NetCDF4 features. Every model file I have ever seen uses only NETCDF4_CLASSIC features.
Disallowing NETCDF4 is not strictly necessary, though, as you can store a "NETCDF4_CLASSIC" file inadvertently as "NETCDF4".
Summarising: Do not support NETCDF4 (-only features), get rid of this check here.
| if auth: | ||
| if username: | ||
| auth.username = username | ||
| if password: | ||
| auth.password = password | ||
| self.url = url | ||
| self.version = version | ||
| self.timeout = timeout | ||
| self.headers = headers | ||
| self._capabilities = None | ||
| self.auth = auth or Authentication(username, password) | ||
| else: | ||
| auth = Authentication(username, password) | ||
|
|
||
| # Authentication handled by Reader | ||
| reader = WMSCapabilitiesReader(self.version, url=self.url, headers=headers, auth=self.auth) | ||
| reader = WMSCapabilitiesReader(version, url=url, headers=headers, auth=auth) | ||
| if xml: | ||
| # read from stored xml | ||
| self._capabilities = reader.readString(xml) | ||
| capabilities = reader.readString(xml) | ||
| else: | ||
| # read from server | ||
| self._capabilities = reader.read(self.url, timeout=self.timeout) | ||
|
|
||
| self.request = reader.request | ||
| if not self.version: | ||
| self.version = self._capabilities.attrib["version"] | ||
| if self.version not in ["1.1.1", "1.3.0"]: | ||
| self.version = "1.1.1" | ||
| reader.version = self.version | ||
| capabilities = reader.read(url, timeout=timeout) | ||
| xml = reader.capabilities_document | ||
|
|
||
| self.WMS_NAMESPACE = "{http://www.opengis.net/wms}" if self.version == "1.3.0" else "" | ||
| self.OGC_NAMESPACE = "{http://www.opengis.net/ogc}" if self.version == "1.3.0" else "" | ||
| if not version: | ||
| version = capabilities.attrib["version"] | ||
| if version not in ["1.1.1", "1.3.0"]: | ||
| version = "1.1.1" | ||
|
|
||
| # avoid building capabilities metadata if the | ||
| # response is a ServiceExceptionReport | ||
| se = self._capabilities.find('ServiceException') | ||
| if se is not None: | ||
| err_message = str(se.text).strip() | ||
| raise ServiceException(err_message) | ||
| self.WMS_NAMESPACE = "{http://www.opengis.net/wms}" if version == "1.3.0" else "" | ||
| self.OGC_NAMESPACE = "{http://www.opengis.net/ogc}" if version == "1.3.0" else "" | ||
|
|
||
| # (mss) Store capabilities document. | ||
| self.capabilities_document = reader.capabilities_document | ||
| # (mss) | ||
|
|
||
| # build metadata objects | ||
| self._buildMetadata(parse_remote_metadata) | ||
| if isinstance(xml, str): | ||
| # (mss) owslib parses with lxml, which rejects str input carrying an | ||
| # encoding declaration. | ||
| xml = xml.encode("utf-8") |
There was a problem hiding this comment.
Parts of this are redundant with the superclasses constructor (compare with https://github.com/geopython/OWSLib/blob/9c94121ca2f6cedf9d50b218a3882c06698c783e/owslib/map/wms111.py#L54, I assume this was copied over from some old version and never updated). The subclass should not repeat what the superclass already does.
|
What would it take to replace MFDatasetCommonDims entirely with |
|
I do not understand this MR. It neither removes the copy-pasting of classes nor exploits it xarray to remove a lot of code. |
|
The issue #2750 this PR belongs to is now divided in 2 new issues (#3184 and #3185). See reason here: #2750 (comment). |
3 participants
Purpose of PR?:
Fixes #2750
Does this PR introduce a breaking change?
added superclass init
If the changes in this PR are manually verified, list down the scenarios covered::
test succeded, not manually verified
Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs
Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes
Checklist:
<type>: <subject>with help by Claude