Conversation
jameshiebert
left a comment
jameshiebert
left a comment
There was a problem hiding this comment.
Overall it looks great. Just a few minor tweaks, and I think we should be good to go.
| python-dateutil | ||
| sqlparse | ||
| testing.postgresql No newline at end of file | ||
| git+git://github.com/karimbahgat/PyCRS.git |
There was a problem hiding this comment.
Yeah, definitely non-ideal to install out of a git repo, but I guess we'll roll with it. Looks like the repo author doesn't have any tags to pin to, but can we at least pin to a specific commit? That way we can ensure that the software doesn't change out from under us (and at least mitigate some of the risk of installing from GH).
| .filter(SpatialRefSys.srtext == | ||
| wkt(cf.proj4_string(var_name, default=default_proj4))) | ||
| ) | ||
| return q.first() |
There was a problem hiding this comment.
I guess if there are multiple matches, we have bigger problems? :)
There was a problem hiding this comment.
Hmm, you're right. I should change that to q.one()
| :param var_name: (str) name of variable for which to insert spatial ref sys | ||
| :return: (SpatialRefSys) inserted record | ||
| """ | ||
| # FIXME: This can cause problems if multiple indexers are being run |
There was a problem hiding this comment.
Really?! Isn't that what ACID databases are supposed to do? Can you roll the search and insert into a transaction to avoid this?
There was a problem hiding this comment.
After reading this I now know why SQLAlchemy transaction management has had me so lost.
There was a problem hiding this comment.
You have no idea how good it is to read that. This took an inordinate amount of time to find a good (I hope) solution for. Yeesh.
| spatial ref sys | ||
| :return: existing or new ``SpatialRefSys`` record | ||
| """ | ||
| spatial_ref_sys = find_spatial_ref_sys(sesh, cf, var_name) |
There was a problem hiding this comment.
I guess you'd actually want the transaction here.
There was a problem hiding this comment.
Addressed by new insert_spatial_ref_sys (transaction is via CTE).
| # responsible for creating it. | ||
|
|
||
|
|
||
| # TODO: Move this out to conftest. How did this even get here? |
| return CFDataset(resource_filename('modelmeta', filename)) | ||
|
|
||
|
|
||
| @pytest.fixture(params=[False, True]) |
|
@jameshiebert ready for next review |
jameshiebert
left a comment
jameshiebert
left a comment
There was a problem hiding this comment.
Just a couple really minor things, but for the most part this looks great! Thanks for taking the time to get it right :)
| # Documentation seems to indicate that ``Session.refresh()`` should do this | ||
| # for us (and more elegantly), but experimentation shows it doesn't. | ||
| sesh.flush() | ||
| # The newly inserted SRS is by definition the one with the highest id. |
| # Miscellaneous constants | ||
|
|
||
| filepath_converter = 'realpath' | ||
| default_proj4_string = '+proj=longlat +ellps=WGS84 +datum=WGS84 +no_defs' |
There was a problem hiding this comment.
This appears to be unused and a duplicate of the default_proj4 variable.
| :return: (SpatialRefSys) inserted record | ||
| """ | ||
|
|
||
| # Use a common table expression (CTE, a.k.a. WITH statement) to compute, |
| numpy | ||
| netCDF4 | ||
| nchelpers>=5.1.2 | ||
| nchelpers>=5.3.0 |
There was a problem hiding this comment.
I'd recommend pinning the version to 5.3.0 here. Or at the very least '==5'. Presumably, we don't want to auto-upgrade major versions which, by definition, have backwards-incompatible API changes
|
@jameshiebert LGTY? |
|
Yep. Looks great. |
2 participants
Resolves #53
Now sets
Grid.srid, which previously was left null, by finding or inserting an entry in the tablespatial_ref_sys(ORM:SpatialRefSys) that describes the coordinate reference system defined in the metadata of the indexed file for the variable for which aGridis being inserted. If no such metadata exists, a default entry corresponding to a PROJ.4 string of+proj=longlat +ellps=WGS84 +datum=WGS84 +no_defsis used.Notes:
This PR updates the ORM to model the table
spatial_ref_sys. This table is created in schemapublicby PostGIS, and the other (modelmeta) tables are created in the username schema (e.g.,pcic_meta). The ORM does not define the schema explicitly because it is different in each modelmeta database. Correctly settingsearch_pathin each database ensures that tables are findable. In the test framework, all tables are in schemapublic.This PR adds a dependency on the PyCRS package, which has a fixed bug that affects us. The only way to get the fix is to install the package directly from the github repo, which seems possibly fragile.