Skip to content

Conversation

@sreeder
Copy link
Contributor

@sreeder sreeder commented Aug 26, 2016

This branch has 4 main changes:

  • Renaming of terms in the ResultValues objects to be consistent with the naming of terms in other objects
  • Updated iPython Notebook (works with new inheritance and minimalist structure)
  • Remove any reference to FeatureGeometry (leave FeatureGeometryWKT). It is causing conversion issues with some of the databases.
  • Remove Geoalchemy as a required library. Since we are not supporting the spatial column, we do not actually need it. If a user would like to add support they can then download geoalchemy, but we don't have it as a basic requirement

@emiliom
Copy link
Member

emiliom commented Aug 26, 2016

@sreeder, let me take a look at this early next week, before you merge into master. I'm currently out of town and on vacation, but I'll be back Monday. Thanks.

@sreeder
Copy link
Contributor Author

sreeder commented Aug 31, 2016

@emiliom have you had a chance to look at this pull request?

@emiliom
Copy link
Member

emiliom commented Aug 31, 2016

Not yet. Sorry for the delay. Tomorrow at the latest.

@emiliom
Copy link
Member

emiliom commented Sep 5, 2016

Ok. I finally was able to look at this. Sorry again for the delay. Pinging @horsburgh so he's in this loop.

I only focused on changes involving FeatureGeometry and related stuff. Specifically:

  • Remove any reference to FeatureGeometry (leave FeatureGeometryWKT). It is causing conversion issues with some of the databases.
  • Remove Geoalchemy as a required library. Since we are not supporting the spatial column, we do not actually need it. If a user would like to add support they can then download geoalchemy, but we don't have it as a basic requirement

I dug deeper into the recent history of commits on models.py. That made me realize that important changes were actually in a commit made back on June 4 that I didn't catch. Among other things, the shape method in the SamplingFeatures class was removed back then. From the perspective of those substantial changes already applied on the master branch, the current PR is just a mop up that makes sense, since it removes references to packages that are not even optionally used, plus FeatureGeometry was no longer even marginally useful.

SO: @sreeder, go ahead with merging this PR. But more broadly, I don't agree with the complete removal of all geospatial capability that started in early June commits. I can definitely see the benefit of making that functionality more optional, including moving related import statements into the optional methods that use them (or even into a utilities module) so that packages like shapely and geoalchemy are not required. But that's different from removing all geospatial capability entirely. FYI, I believe Choonhan was relying on the shape function for ODM2RESTAPI, and I personally found it very useful and non-obtrusive, except requiring the installation of those geospatial packages.

I'll open an issue to help me/us address the broader issues later on, w/o impeding this PR.

@sreeder sreeder merged commit f2bd62f into master Sep 27, 2016
@sreeder
Copy link
Contributor Author

sreeder commented Sep 27, 2016

@emiliom I have forgotten what I need to do to prompt a new release of the api to be created for the conda channel. v0.5.2.

@sreeder sreeder deleted the renamevalueobjects branch September 27, 2016 19:36
@emiliom
Copy link
Member

emiliom commented Sep 27, 2016

For now, the best way is to open an issue on the conda-recipes-ODM2 repo requesting the update via a very brief message.

Here's a good example, from a closed issue requesting an odm2api update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants