Conversation
… version of zarr to setup. added missing webob dependency to setup
… tracking (was for debugging)
…et_data to prevent inadvertent writing attempts. changed exception catching in utils _get_from_url.
…ource to use slice coordinate_index_type to avoid incompatibility with numpy type.
…cache clearing function. Removed DiskCacheMixin. Removed lazy loading of pydap because of xarray pydap.__spec__ interaction. Fixed missing source_coordinates usage in interpolation_manager._fix_coordinates_for_none_interp.
|
mpu-creare
left a comment
mpu-creare
left a comment
There was a problem hiding this comment.
Couple of minor comments. Mostly looks good to me.
| raise ValueError("Unknown cache store type '%s', options are %s" % (name, list(_CACHE_STORES))) | ||
|
|
||
| return CacheCtrl([_CACHE_STORES[name]() for name in names]) | ||
| cache_stores = [] |
There was a problem hiding this comment.
| cache_stores = [] | |
| # makes all requested cache stores and fails gracefully if one of the stores is unavailable | |
| cache_stores = [] |
There was a problem hiding this comment.
I got the approval notice before these suggestions and jumped the gun a bit. I'll get this into the branch that is still merging into main.
| # ------------------------------------------------------------------------- | ||
|
|
||
| def open_dataset(self, f): | ||
| return pd.read_csv(f, parse_dates=True, infer_datetime_format=True, header=self.header) |
There was a problem hiding this comment.
What was the problem with infer_datetime_format ? New pandas behavior? Seems like a potentially important flag...
There was a problem hiding this comment.
For the version of pandas that was installed when pip installing podpac, that argument no longer existed.
I suspect me using python 3.11.12 for this effort may have caused me to do some unexpected "newer python" updating. This and the "zarr<3" in setup seem to both be python version upgrade-like changes.
| """ | ||
| # get data from data source at requested source coordinates and requested source coordinates index | ||
| data = self.get_data(rc, rci) | ||
| data = deepcopy(self.get_data(rc, rci)) |
There was a problem hiding this comment.
Why the deepcopy? that's rarely needed...
There was a problem hiding this comment.
This later line
udata_array.data[np.isin(udata_array.data, self.nan_vals)] = self.nan_val
would get "read-only" permission errors if the original get_data was used for one of the unit tests.
|
|
||
| fp.seek(0) | ||
| rnode = Rasterio(source=fp.name, outputs=node.outputs) | ||
| rnode = Rasterio(source=fp.name, outputs=node.outputs, crs="EPSG:4326") |
There was a problem hiding this comment.
The CRS should be picked up automatically from the file -- what was the issue here?
There was a problem hiding this comment.
For these unit tests, if crs wasn't specified, Rasterio would end up with this very verbose CRS, and I'm not sure where it was coming from.
| def test_raise_requests_error(self): | ||
| mock_requests = MagicMock() | ||
| mock_requests.get.side_effect = ConnectionError("Test Connection Error") | ||
| mock_requests.get.side_effect = Exception("Test Connection Error") |
There was a problem hiding this comment.
ConnectionError seems like a better Exception type -- why the change?
There was a problem hiding this comment.
Pytest claimed that it was improper to catch something that did not inherit from Exception and would fail the test
There was a problem hiding this comment.
REally?. I just checked the 'requests' code, and it seems like the inheritance is Exception --> IOError --> RequestException --> ConnectionError... so it DOES inherit from Exception.
Also
import requests
assert isinstance(requests.ConnectionError(), Exception)| _log.warning("Cannot connect to {}:".format(url) + str(e)) | ||
| r = None | ||
| except RuntimeError as e: | ||
| except Exception as e: |
There was a problem hiding this comment.
Same comment about ConnectionError being a better exception type.
There was a problem hiding this comment.
Because pytest wouldn't permit the ConnectionError, it meant more than just RuntimeErrors could be raised. This change was to handle that, and acknowledged that only the wording of the error message changed with the different except cases.
| # lazy_class("pydap.__spec__") | ||
|
|
||
| # Lazy loading was conflicting with xarray access of pyap.__spec__ | ||
| import pydap |
There was a problem hiding this comment.
I just realized we can still make pydap an optional dependency using the pattern:
| import pydap | |
| try: | |
| import pydap | |
| import pydap.model | |
| import pydap.client | |
| except: ImportError | |
| class Dum: | |
| pass | |
| pydap = Dum() | |
| pydap.model =None | |
| pydap.client = None |
2 participants
Merging updates from the release branch back into develop