Skip to content

add geometry column to sites RM-1694#96

Merged
gsrohde merged 6 commits intomasterfrom
RM-1694-geometry-column
Jul 31, 2014
Merged

add geometry column to sites RM-1694#96
gsrohde merged 6 commits intomasterfrom
RM-1694-geometry-column

Conversation

@robkooper
Copy link
Copy Markdown
Member

  • Add geometry column to sites, initialized with data (lat, lon, masl)
    [masl == elevation]
  • All sites are set to have srid/espg=4326 (WGS-84)
  • Need to switch database.yml to use postgis instead of postgresql

- Add geometry column to sites, initialized with data (lat, lon, masl)
[masl == elevation]
- All sites are set to have srid/espg=4326 (WGS-84)
- Need to switch database.yml to use postgis instead of postgresql
Comment thread Gemfile
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to keep mysql around, now we are using postgis features such as geometry.

@robkooper
Copy link
Copy Markdown
Member Author

@dlebauer @phenolphtalein @gsrohde this was asked by david for the next sprint.

@dlebauer
Copy link
Copy Markdown
Member

@phenolphtalein can you please review this pull request? and then reassign to @gsrohde once all of the migrations work and tests pass?

@phenolphtalein
Copy link
Copy Markdown
Collaborator

@dlebauer @robkooper I had to install the 'spatial_adapter' gem to make it work, otherwise rails doesn't seem to be able to get xyz coordinates from the point. Do you get the same problem?

@robkooper
Copy link
Copy Markdown
Member Author

You should have been able to do a bundle install since the requirements are in the GemFile.

@phenolphtalein
Copy link
Copy Markdown
Collaborator

It wasn't in the Gemfile, there is only postgis-adapter.

@robkooper
Copy link
Copy Markdown
Member Author

mmm, ok. I didn't install it explicitly maybe different versions of ruby. One thing that I did (I just realized) was the following commands to convert the database to have the postgis extensions:

rake db:gis:setup
and/or
CREATE EXTENSION postgis;

@phenolphtalein
Copy link
Copy Markdown
Collaborator

Yes, I did that but still needed the gem to for the functions to work properly.

- add index to geometry
- use centroid if not a point
- remove activerecord-postgresql-adapte, is replaced by
activerecord-postgis-adapter
@robkooper
Copy link
Copy Markdown
Member Author

updated pull request to add code to return centroid for lat/lon/elev if geometry is not a point.

@gsrohde
Copy link
Copy Markdown
Contributor

gsrohde commented Jul 14, 2014

Rob: Mike closed the corresponding Redmine issue without this being pulled. If you still want me to pull this, then either (1) re-open the Redmine issue, assign it to me, and set the category to "Pull Requested", or (2) Re-assign this pull requested to me, preferably with a comment like "this pull request has been checked by ... and is ready to go". (Preferably, do both of these! We have option (1) partly because the undergraduates don't have the permissions needed to assign Github issues.)

@gsrohde gsrohde assigned robkooper and unassigned phenolphtalein Jul 14, 2014
@robkooper
Copy link
Copy Markdown
Member Author

@phenolphtalein is pull request ok? Can somebody else and try if they have same problem with missing gem?

@phenolphtalein
Copy link
Copy Markdown
Collaborator

@robkooper @gsrohde I thinks it's ok. Everything worked fine after adding the gem.

@robkooper
Copy link
Copy Markdown
Member Author

@gsrohde @dlebauer what is the status?

@robkooper robkooper assigned gsrohde and unassigned robkooper Jul 18, 2014
@dlebauer
Copy link
Copy Markdown
Member

@robkooper

From Scott's reply above:

If you still want me to pull this, then either (1) re-open the Redmine
issue, assign it to me, and set the category to "Pull Requested", or (2)
Re-assign this pull requested to me, preferably with a comment like "this
pull request has been checked by ... and is ready to go".

On Friday, July 18, 2014, Rob Kooper notifications@github.com wrote:

@gsrohde https://github.com/gsrohde @dlebauer
https://github.com/dlebauer what is the status?


Reply to this email directly or view it on GitHub
#96 (comment).

@robkooper
Copy link
Copy Markdown
Member Author

Assigned to scott. don't want to assign it to scott in redmine since he has not done anything on and should not get the credit in redmine for it.

@dlebauer
Copy link
Copy Markdown
Member

If anyone is using Redmine to assign credit for work done, such use should
not interfere with it's use as a task manager. Assigning to Scott with the
category "pull requested" is a clear indication of his role in the process.
The problem is a result of using both redmine and github to manage the
workflow, since only people with write permissions can assign a github
issue.

On Friday, July 18, 2014, Rob Kooper notifications@github.com wrote:

Assigned to scott. don't want to assign it to scott in redmine since he
has not done anything on and should not get the credit in redmine for it.


Reply to this email directly or view it on GitHub
#96 (comment).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this mysterious number 4326? Even though this is just a migration, I think this should be a named constant, both here and in the Site model, and it probably should have a comment explaining it if the name of the constant doesn't say it all.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about the mystery numbers, even when googlable, they shouldn't need to be googled by everyone reading the code ... the following info should be in the comments

Spatial Reference System Identifier (SRID) http://en.wikipedia.org/wiki/SRID
SRID 4326 is WGS 82 http://spatialreference.org/ref/epsg/wgs-84/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added info

Comment thread Gemfile
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this replaces and extends activerecord-postgresql-adapter, correct?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct

@gsrohde
Copy link
Copy Markdown
Contributor

gsrohde commented Jul 24, 2014

I was able to run the migration after running psql -d bety -c "CREATE EXTENSION postgis;" but when I try to visit the sites page, I get undefined method 'geometry_type' for #<String:0x007fdfabac2028>. Please add to the bety wiki setup instructions (and if need be, the pecan wiki as well) with any additional postgres setup that needs to be done to support these changes. This should include both instructions about updating an existing installation and setting up a new one.

@dlebauer dlebauer assigned robkooper and unassigned gsrohde Jul 24, 2014
@robkooper
Copy link
Copy Markdown
Member Author

Yes you do need to add the postgis extension. I loaded the ebi-forecast database for a fresh start. Only problem was the create extension, once that was executed the migration worked and no problems encuntered, The sites are listed normally.

@robkooper robkooper assigned gsrohde and unassigned robkooper Jul 28, 2014
@gsrohde gsrohde merged commit ca957ac into master Jul 31, 2014
@robkooper robkooper deleted the RM-1694-geometry-column branch May 1, 2016 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants