Gazetteer model

[kleintom]

I'm submitting a first draft of

• a somewhat reworked GeographicItem model
• a new Gazetteer model
• a 'New Gazetteer' task

All of those are unfinished (and possibly somewhat broken), but I wanted to put something out to make sure I'm on the right track. All/any feedback welcome, but I'm not looking for a full review at this point - I consider this all to be in the early stages, so 'big' changes are fine with me.

Some notes/issues/questions I've encountered so far:

• I doubt all the specs pass, though GeographicItem currently should. GeographicItem specs have been duplicated, but I haven't changed the second copy at all yet to cover the new geography column.
• Some changes in GeographicItem:
  • shape_column_sql(shape) returns, e.g. the polygon column if that's present, or the geography column if that column holds a polygon. This will replace many of the existing CASE statements once everything switches over to a single geography column.
  • More st* methods for working with st* statements (feedback welcome), see also select_self.
• In GeographicItem I changed all ST_Contains to ST_Covers. I couldn't tell why one was meant over the other in particular cases, and ST_Covers seems easier to reason about (to me). (The difference is ST_Contains doesn't contain things in its boundary, so e.g. a polygon wouldn't contain its vertices, etc.) That could be a breaking change, and would be easy to revert.
  • One question would be which should be used in new code (maybe it depends on situation).
  • If there's no builtin reason for one over the other then a couple other spots in the code could be changed to ST_Cover instead of ST_Contains, if we wanted to settle on one over the other.
• There are nearly 500 lines of code in GeographicItem that are either
  • not referenced at all
  • are only used in specs to test themselves
  • are only used in specs to test other things I put all of those in a separate GeographicItem::Deprecated module for now, but it's still included in GeographicItem (for now) and I don't have any real opinion on whether that's the way forward or not.
  • Personally my thought was to put the used-only-for-specs methods somewhere else where they would only be visible in specs, but I wasn't seeing any clean way to do that.
  • I am now assuming that PostGIS >= 3.0 so that we should be able to handle all GeometryCollections in ST_* functions.

The 'New Gazetteer' task allows the user to add multiple shapes to a new gazetteer before saving (i.e. they can draw multiple shapes in leaflet, for example, for one gazetteer) - they all get bundled into a single collection behind the scenes. That differs from the georeference model I think, but fits more closely with GeographicArea shapes (I think) - this could also be a path to combining existing GeographicItems into a Gazetteer, depending on how we want to store that. Leaflet shapes and WKT shapes (and eventually other input methods) can be combined in one gazetteer - I don't keep track of which shapes came from which input types on the backend.

• One question is how to deal with intersections of multiple shapes. I'm not sure if that may cause issues or not. One leaflet polygon drawn inside another combines to a donut (inside leaflet iirc), which is probably a useful thing, allowing users to create donuts, but maybe in other cases it's unexpected/more tricky/doesn't-combine-well-with-wkt, I'm not sure.
• I've turned off circle creation in leaflet for now at least - we'd need to approximate by a polygon on the backend I think, which is a whole thing (is it already covered in a library somewhere?).

As @mjy suggested, I've gone with a write once read/replace pattern for the geographic item shape of a gazetteer.

I've yet to take any position on meaning of Gazetteer parent in the code.

Most of the crud create/list/show stuff should be mostly working, but it's not all finished yet.

[mjy]

Thanks @kleintom

I changed all ST_Contains to ST_Covers

I feel this is a safe assumption.

which should be used in new code?

This is essentially everything but me vs everying AND me. I feel most will want everything AND Me (covers) so I would go ahead and make covers the primary workhorse in new code. There should be decent spec coverage on this, if there is a lot on contains version then it suggests that it was important too...

There are nearly 500 lines of code in GeographicItem that are either ... not referenced at all

Rip it out. Make a single commit that eliminates it and the corresponding specs so that we can diff against it down the road. A bigger challenge is to eliminate all the items we don't test against as well (support/geo stuff). Let's not spend time on understanding code we don't use, or might use, let's clean slate things.

One question is how to deal with intersections of multiple shapes.

I think you're asking whether we should compute and store a single shape, or store all the shapes. My initial thought was compute an intersect before persisting. Perhaps a virtual attribute ('combine shapes'). I guess we have the flexibility to make this optional. I think performance wise combining selected shapes into one, and possibly simplifying them as well, might be desirable options. A major use case is to define, by selecting multiple shapes a squared off printable base-map, with borders roughed in.

all get bundled into a single collection behind the scenes. That differs from the georeference model I think,

Not really. Individual Geoferences can each be cited and annotated, we don't want the shapes from individually performed Georeferences to be combined in the back end.

I've turned off circle creation in leaflet for now at least - we'd need to approximate by a polygon on the backend I think, which is a whole thing (is it already covered in a library somewhere?)

Hmm. This is a major consideration. Perhaps we will have to store a radius value for retroactive edit/adjustment functionality, and use a before hook to calculate the shape?

[jrflood]

It may be worth considering introducing the ordered bounding box (á la OvertureMaps) in GeographicItems (and maybe also GeographicAreas) to provide a quick (maybe orders of magnitude faster) selection of different Venn cases in searches. Early Taxonworks overlaid results with a bounding box that was flawed especially when dealing with antimeridian spanning shapes -- not then recognizing (or correctly calculating) the necessary west to east ordering.

[kleintom]

The main new thing here is specs for the new geography column of GeographicItem. There are a couple significant departures from existing GeographicItem specs (though for the most part the things being tested are the same), suggestions/feedback welcome.

• Fewer, simpler shapes, more descriptive names
  • fewer shapes in the sense that, e.g. a single polygon can provide multiple shapes: a polygon, lines, points, multilines, multipoints that can participate independently in specs as needed. This makes shapes more tightly coupled, but reduces the number of shapes to keep track of. I'm open to thoughts on which is preferred, or if there's a better option.
  • Simpler axis-aligned shapes. If specs pass for those but fail for more complicated shapes, my take is that that's an rgeo/postgis/geos issue. Axis-aligned along the "equator" also allows for more precise distance specs (though again this couples the existing shapes to be at certain distances).
  • More descriptive names: the hope is that one can, more or less, read the new specs without looking up what the shapes are. I may have gone overboard in this respect (especially for someone without autocomplete), some names are quite long.

Other updates/changes:

• geographic_item now has one gz rather than many. Am I missing a reason that shouldn't be the case? Oops, see the next point.
• I removed the (unimplemented) clone option that I copy-pasted there, and then I remembered this comment, regarding sharing across projects:

  Finally (and this is where I would lean to start) we could simply clone Gaz entries, while pointing to the identical shape. This would permit discovery and forking of use.

Do you mean users in one project could somehow see/search gzs in another project and clone the ones they want into their project?

May also need to clone (GeographicItems from) GAs if we're going to keep track of the separate GAs we're using to form a union (see also comments below).

• I can't get leaflet to produce donuts anymore - I thought at one time it was doing that when I drew one polygon inside another. You can still create a donut via wkt, maybe that will be the only way (as it is for georefs I think).
• Saving a shape that crosses the anti-meridian fails in the expected way, that will need to be fixed.
• All of the crud stuff should be working now
• When you save multiple leaflet shapes/wkt shapes/etc they're now unioned on the back end (instead of collected). Depending on how other issues mentioned here work out, this may need to be adjusted.
  • pros: simplicity, performance
  • cons: you lose track of your original shapes, harder to read the combined shape in some cases

    A major use case is to define, by selecting multiple shapes a squared off printable base-map, with borders roughed in.

I assume you mean selecting multiple separate gzs(/gas/grs?), not multiple shapes from those within a single gz?

• As a first step I added a kind of support for circles - a circle drawn in leaflet gets converted to a polygon on the back end and forgets it was ever a circle (which means it no longer gets reported as a circle in the ui). Number of polygon segments is configurable. I think georef radii are limited, should there be a similar limit here?

  Perhaps we will have to store a radius value for retroactive edit/adjustment functionality, and use a before hook to calculate the shape?

I'm probably overemphasizing the ability to add multiple shapes to a gz, but a couple thoughts: if we only want to allow one circle then we can just have a gz radius column, if we wanted to allow multiple then we need a separate circles table (I think). If we just add a radius column then we need some way to associate the radius with a particular point within the collection of shapes for that gz (some of which may be other points), so probably better to have a separate circles table (with radii and centers), or only allow a single lone circle shape if you want a circle. This would all require keeping shapes separated (not unioned), or at least keeping circles separate. That could all be done though I think.

Next steps for me:

• Add filtering on a gz
• Add import via shapefile/some other means

Other remaining issues:

Parentage needs much cleaner semantics in Gaz, if indeed we use it at all (is it geopolitical, spatial, set defining). Let's not try to sync it with GA for now, because there it means "as used in TW to build the Gaz, roughly geopolitical sets", which could be better defined.

I see what you mean now, I'll go with whatever you think here.

• Forming unions of GAs and/or other gzs. Implementation to be decided, see comments above.

[mjy]

A little review/thoughts on the present state:

IIRC the strategy is to land this PR then do a followup where we merge everything down to the single geography column, correct? The following expands on that thinking-

• Let's not try to add too much more CASE/conditional handling for the multiple column types. Since we're assuming we're going to merge everything into a single column with a data migration the PR "after" this, we can minimize the exception handling work at this point.
• I think you can create a unifying migration to copy the existing data columns to the new column. We'll leave the data in place in the existing columns and include a cleanup commit after this is merged.
• Do we need a sublcass of GeographicItem if it is going to move to a single column? Maybe not?

Fewer, simpler shapes, more descriptive names

This looks great, thanks.

Do you mean users in one project could somehow see/search gzs in another project and clone the ones they want into their project?

Something like this, yes. For example there are people with multiple projects, they might want to "copy" their Gazs b/w them.

When you save multiple leaflet shapes/wkt shapes/etc they're now unioned on the back end (instead of collected).

Thanks for pointing out the pros/cons. I agree, let's see what kind of responses we get as people begin to use the Gaz.

I assume you mean selecting multiple separate gzs(/gas/grs?), not multiple shapes from those within a single gz?

Yes!

As a first step I added a kind of support for circles...no longer gets reported as a circle in the ui)

Seems OK for now, again we can wait to see how this plays out. I don't anticipate a lot of circle use, but I could be wrong. Thanks for the observations on multiple circles. I think we're talking significantly more complexity than we want to initially introduce in that scenario. Our basic idea is not editability, it's stored shapes. We can argue that those shapes are best defined in interfaces that edit well, our UI can be "rudimentary".

[kleintom]

The main new things in the latest batch of commits:

• Added a filter-on-gz facet for ce's, co's, otu's, and ad's (everywhere there's currently a ga facet I think).

  • in the gz case the Descendants and Exact options don't apply (at least not yet), so there's only the spatial option.

• One important point I overlooked in the new gi::geography specs is that, for example, I defined box_centroid in terms of box, which means anytime you instantiate box_centroid you also instantiate box. That defeats the idea of being able to use these shapes independently, so now all shapes are defined in terms of independent geometric data sufficient to describe the base shape (like box). Multi-shapes still instantiate their constituent shapes.

• Rewrote GeographicItem ST* methods using Arel. See if that's what you had in mind - I think there was a little bit of Arel creep, in the sense that I had to Arelize in a couple places where I was calling into an ST function (I think, not sure), but overall it does feel a lot more solid. (Any comments on the state of Arel regarding the possiblity of breaking changes in the future? The benefits it provides now outweigh that possibility?)

• Added the input-a-point option for the New Gz task - I'm pretty much trying to follow CE GR input here, which also has the geolocate option. Is the geolocate option something you'd like to have for GZs as well? (I still need to add the iso_a34 input stuff in any case.)

• Added a new Import Gazetteers task. Right now it supports importing from a shapefile; that should basically work, but it it's not built out yet. (Would you like other import options at this point?) I think there's still quite a bit to settle on there:

  • It's seeming like this import needs some kind of progress indicator; you may be able to just update the GZs list until you see your last shapefile Gz if we decide not to do everything in a transaction (see next point), but that's really not that great in any case. The solutions I've seen/considered all seem to use either extra columns or a separate table that background jobs can write to to poll from - the polling could involve actioncable, but so far at least it seems like the extra columns/table would be necessary. Any thoughts on solutions (has dwcimport already done this?), or could I think a little more about a proposal to present?
  • Should the entire import fail if one record import fails? Right now I'm ignoring record fails due to bad shapes or bad names (i.e. not present), but other failures cause a transaction abort; that probably won't be where things stay though. Currently things run in the foreground (will change to background - the Ecoregions file took 1 hour 20 minutes on my desktop for 847 records) and shape and name errors are reported after import. Issues:
  • It might not be so easy to tell if your shapefile has "clean" shapes until you try to import, especially if you just downloaded it from somewhere. e.g. the Ecoregions shapefile (I think I used this one: https://www.geographyrealm.com/terrestrial-ecoregions-gis-data/ ) had quite a few self-intersections; after I ran it through ogr2ogr -f "ESRI Shapefile" -makevalid fixed_shapefile.shp Ecoregions2017.shp there were still two errors on import, even though ogr now said the file was valid - both failing shapes were anti-meridian crossing, it seems rounding issues using our factory are turning longitudes like 179.99993158 (meant to skirt the anti-meridian) into -179.9999999999, and then those shapes get self-intersections and can't be imported.
  • Should the entire import fail in that case? I have two possible alternatives in mind, can you think of others?
    • support a "test run" option, like a sandbox that runs in a transaction, collecting failure information as it goes, and then aborts the transaction at the end (or maybe doesn't abort if there were no errors?). Reporting the errors would be an issue though, a progress indicator/similar would have to display those when the job ends, or as they arise (right?).
    • another option might be a checkbox saying Ignore shape failures, but that could be troublesome - e.g. here the original shapefile had many bad polygons, if I import the "good" ones, then see how many bad ones there were and run ogr to fix them, now I have no good way to reimport without duplicating the original good ones. On the other hand if I don't allow that then I'm left trying to fix the two errors above by hand, or removing them from the shapefile (ST_Valid returned true on them iirc, it was only on the import rounding that they became invalid, and then it was too late because the record import failed before their geometry could even be examined or fixed). Maybe a stern warning would be fine.
  • Another issue around failures is names - users have to provide the name of the name column (we're not guessing); if they enter a typo then every record will fail. This can at least be checked before the background job is kicked off; if the first 5 records fail on name assume a typo. But if we think some shapefiles might have multiple empty names and we want to worry about that then we'll have to decide how to deal with it.
  • Apparently rgeo-shapefile uses whatever projection is defined by the factory provided when the reader is opened. For us that's wgs84, so every record is assumed to have wgs84 coordinates (I'm not sure how the mercator projected projection of our factory fits in here). Right now a prj file is required, but I'm thinking that will be dropped with a note that wgs coordinates are required, and if prj is provided we can confirm or raise error. (Again if people assume wgs84 and then see their shapes are all wonky when they import, how can they delete all of them except one at a time?)
  • Would it be helpful to add an import source column as in GA? Maybe autocomplete could include it as a way for users to try to figure out if the shapes from a particular shapefile/source have already been imported? And/or add a filter gzs task to provide other similar means, possibly along with bulk delete (on semi-failed/completely-failed imports).

  Left to do:

  • Migrate to geography column, associated cleanup
  • Support creating gzs as unions of gas/gzs. Three options I can think of, maybe you'll have more:
  • Just combine everything on the backend, same as it is now for other added shapes; I think this would be simplest, though you lose track of what the union members were
  • Use a combination of leaflet/wkt/point-unioned shapes and closure_tree union members (via parent-child relationships); leaflet/etc shapes could be combined and stored as they are now, and then on access that would be unioned with child gzs on the fly (does that make sense?).
  • Allow the union, but dont allow other leaflet/etc shapes in that case. Parent can only be sti-polymorphic in closure_tree, so i think you'd either need to clone GAs into GZs to support unions, or add a polymorphic reference on GZ which could point to a GA or GZ for use in unions - I guess that option would make it easier to support unioning with other GZs (otherwise the child Gz would need to be cloned in some way from a Gz at some other location in the tree). Again the union would need to be created on the fly (right? or cached?).
  • Plenty of cleanup left to do.

Thanks!

[kleintom]

• It might not be so easy to tell if your shapefile has "clean" shapes until you try to import

I noticed RGeo::Shapefile does have a allow_unsafe: true option to open (documented in the readme but not on the open rdoc) which allows you to read invalid shapes. You can then rgeo make_valid them, though it's not a fast process on large shapes (maybe there would be some limit on the number of shapes that could be fixed before giving up?).

• Right now a prj file is required, but I'm thinking that will be dropped with a note that wgs coordinates are required

Now I'm thinking a proj file should be required...

Maybe given those two changes there would be less chance for mass numbers of records failing and less need for test runs/mass deletes/progress reporting errors etc. that I suggested as options above.

• Would it be helpful to add an import source column

Then GZs could be unique on name + import source and different shapefiles could import shapes of the same name.

[mjy]

in the gz case the Descendants and Exact options don't apply (at least not yet), so there's only the spatial option.

Let's keep this basis for the original implementation I think, i.e. spatial considerations only.

One important point I overlooked ... I defined box_centroid in terms of box, which means anytime you instantiate box_centroid you also instantiate box ... Multi-shapes still instantiate their constituent shapes.

This should be fine. For each model we have a valid_<model> factory so that when writing specs devs can immediately know that something like FactoryBot.creat(:valid_otu) exists and will work. It feels like this concept should be in place for the shapes as well, i.e. if I need a valid multipolygon there is a convetion to immediately get one. An issue with the previous code was that there were many many of each shape pre-coded/named. As long as we're moving towards instantiating multiple "similar" shapes within the spec context itself, as needed (somewhat less DRY, but more immediate) I think it's headed in the right direction. More perhaps when I further grok the code.

Any comments on the state of Arel regarding the possiblity of breaking changes in the future? The benefits it provides now outweigh that possibility?

This is an excellent insight/question. Here's how things have evolved AREL wise thus far for background. As predicted and described by various rails devs and others we hit a point where the ActiveRecord (AR) abstractions required alternate approaches for certain things, the common branch is to add a new gem, write raw SQL, write your own abstraction layer (see Filters later on), or sprinkle in AREL. I did a little of the latter, then when experimenting with approaches to merge queries and writing more complex SQL started moving into heavy use of AREL. In part this was to not write literal SQL, or to minimize that, that felt more "native". I also had an incorrect, I think, view that merging queries was going to be easier/possible if everything was AREL. I've since seen that the UNION/INTERSECT etc. patterns, and other abstractions in filters handle this pretty nicely, so the amount of AREL I've written has dropped dramatically. There are schools that say you shouldn't write any AREL as it's API is intended to be abstracted away. The question that you raise is spot on then, if that layer is abstracted out for something else what is our cost going to be? My gut feeling is a) it's not going away soon; b) if we use it to instantiate single methods, rather than the complex patterns of joining multiple SQL statements as we have some examples for, that we're adding minimal refactoring risk. It might be grindy to fix, but not mentally challenging. Doubly OK if we have specs for the methods. So TLDR, you're right on to be cautious IMO, but I think we understand the risks/rewards ... I think :).

Is the geolocate option something you'd like to have for GZs as wel

Good question. It's probably going to be useful, and we're likely able to abstract the code to serve both models if so. I think there are other efforts that might be more useful to wrap emerging as well. None of this is critical for the initial release.

SNIP questions regarding multi-shape import, addressing at another point

Would it be helpful to add an import source column as in GA?

We do have one precedent in TW, the origin_ columns in GA/GAGI. I'd rather not repeat this here though, and just use Citations and potentially DataAttributes. The latter express all the semantics we need. "If I find this Source then I can reasonably expect to find the thing (Gaz) it is Citing therein.". So, let's just make it easy to Cite the GAs.

... support creating gzs as unions of gas/gzs. Three options I can think of, maybe you'll have more: Just combine everything on the backend, same as it is now for other added shapes; I think this would be simplest, though you lose track of what the union members were

That's OK, I'd do this first (and in fact I think the other approaches you mention are too complicated, at least that's my gut feeling). The point is that people are going to be conceptually thinking of this as one thing for the most part. The paradigm we want to teach, initially, is "forge your shape elsewhere, when you are happy with it add it as a single entity here". We don't want to rebuild QGIS in app. We want people to reference a single space on earth here. Write once, read many. If you need to tweak, write another, read many.

...

Back to multi-shape import. You're anticipating a lot of generic needs, and things we've done elsewhere. Perhaps the most efficient approach is for your to experiment somewhat extensively with the DwC importer, to see what we did there. It's a major task to work out down the road, but ultimately we want to abstract that UI and code to handle different data types. There is no reason IMO why we couldn't handle the much simpler data we have in GAZ in this framework. It has many very nice features that you anticipate needing. So- I would say, again, for the initial PR let's not make multi-import a must-have, but rather if we can handle a simple format that we can do some very quick cross-shape validation on and return some very strict yes/no we can do this I think it's worth doing. Implementing something more complex with all the features you mention might be better served by taking the time to abstract the DwC importer. If you need help finding a DwC file to play with (there are some sandboxes with many of them) let us know.

[mjy]

@kleintom Peeking back at this. Let me know if I can help out in some regard. Again, not critical, just seeing if we can facilitate. Thanks!

[kleintom]

@kleintom Peeking back at this. Let me know if I can help out in some regard. Again, not critical, just seeing if we can facilitate. Thanks!

Hi Matt, thanks for the ping and the offer - I think things are close, I expect to be ready for review sometime in the next week.

[kleintom]

It's been awhile, so I'll try to summarize the main changes and issues here:

• GeographicItem has been refactored, the main changes:

  • there's now a single geography column that supports a shape of any type (point, polygon, etc) rather than a separate column for each type
  • a migration copies the existing GeographicItem shape to the new geography column without deleting the existing shape columns (deletion will occur in a future pr)
  • all code referencing the old type columns is gone, GI is no longer STI
  • all unused GI code has been removed, ~500 lines iirc
  • a new basis of st_*_sql methods has been created to support a cleaner interface for code using the postgis ST_* methods
  • raw pg code has largely been replaced with Arel NamedFunctions which are more cleanly composable and provide more checks
  • geographic_item_spec.rb has been rewritten with a simpler, hopefully easily digestable suite of shapes (other geo test files have not yet been changed)
  • the new GI code has switched to just the simpler-to-reason-with ST_Covers instead of mixed use of that and ST_Contains - it's possible there will be some edge cases where behavior has changed (I did try to add specs to document at least some of those edges).

• A new Gazetteer model has been added, essentially these are user-creatable named shapes. They also hold optional iso_3166 a2 and a3 values, but there's (currently at least) no hierarchical relationship between Gazetteers based on any of these data - each Gazetteer exists as an independent name/shape.

  • Currently the main use is for filtering geo data: asserted distributions, collecting events, collection objects, and otus.

• A New/Edit Gazetteer task

  • Create by:
  • drawing shapes on leaflet, including circles
  • entering WKT, point coordinates
  • adding existing geographic area shapes and/or existing gazetteer shapes
  • TODO? Add Geolocate entry option as in georef?
  • On submission all of those shapes get combined together into a single shape that is then no longer editable - no record of the original input shapes is kept. That final shape is made valid if it's not already (more details further down).
  • There's a preview checkbox so you can check on what your final combined shape will look like on the leaflet map before you submit

• An Import Gazetteers From Shapefile task

  • Add the individual files from a shapefile to TW as Documents (zipped shapefile not supported, but you can multi-select all of your files at once in the New Document selector in the task): shp, shx, dbf, and prj are required, cpg is optional but should be included if it exists; any other shapefile files are ignored and don't need to be uploaded.
  • Only shapefiles in the WGS 84 (EPSG: 4326) Coordinate Reference System (CRS) are supported.
    • TODO? If a prj file contains an epsg id for its CRS then it may be possible, at least in some cases, to convert to WGS 84 on the fly (but my impression so far is that most prj files don't actually include that??)
  • All relevant documents need to exist in TW, but the interface only requires selecting the .shp file - the other files will be found by name (unless there are duplicates (which I think is the only reason the document selector needs to be able to work with more than a .shp file)).
  • Gazetteers require non-empty names; you can either enter your shapefile's name field by hand or click to select it from a list of all of the shapefile record fields.
    • TODO restrict that list to only those columns that hold Strings.
    • Currently every record is required to have a non-empty name value - the job won't run unless that's the case. That could be adjusted.
    • One thing we can't do anything about is the possibility of the user accidentally selecting the wrong name field and then potentially importing many useless gazetteers.
  • Shapefile processing runs in a background job. Large jobs can take hours, so there's a GazetteerImport model that tracks the status of each job and displays that status in the task.
  • Up until a few days ago I meant for the job to run in a transaction: either all records would be converted correctly to Gazetteers or none would. I came to discover that doing that with long jobs with lots of records was a memory sink (see more comments in the commit that changes that behavior), so now the known errors that can occur during processing are caught and reported in the status of the job and the job continues (unknown errors end the job early). That means that shapefile records with no issues get imported, those that have some issue do not get imported but you should get some idea of what went wrong in the ui (this is not a great solution currently if there are lots of errors).
    • I feel much better now about known shape errors and dealing with those as best we can, but shape errors that do occur may require some more specialized knowledge to fix and rerun without simply reimporting the whole file and creating duplicates of those already successfully imported in the initial run.
  • All shapes to be imported are checked for validity; if an invalid shape is encountered it's make_valid-ed (can that fail?) and the valid version is imported.
    • TODO make_valid can introduce lower-dimensional artifacts which should be removed (I think).
    • The shapes known to fail with current processing are things like invalid shapes that also cross the anti-meridian - see below. The other known failure is when RGeo::Shapefile tries to load a troubled shape that Geos raises on before we ever even get a reference to it - n.b. those shapes can be valid according to both RGeo and postgis!
  • I haven't looked into DWC's imports yet since I was thinking in transaction terms, but now that there's not a transaction that may be a possibility.
  • I added the ability to choose to import your Gazetteers on creation into any multiple projects you're a member of - let me know if that's not what you'd like and I can pull it out (or just hide it for now).
  • The projects imported into are listed in the jobs status UI, but only in the project you imported from, not in all projects you imported to.
  • Either a given GZ will be saved to all projects or none (that part does run in a transaction).
  • There's currently no way to clone an existing GZ into another project (but that's certainly doable).

• A couple of minor nagging issues:

  • When you drop multiple files into the New tab of the document selector for import, sometimes not all get imported, sometimes the page reloads, sometimes the import seems to import some files, hang for 10-15 seconds, and then complete, sometimes it all works fine. (I feel like this has somewhat improved recently though?)
  • Sometimes shapes drawn in leaflet disappear when you pan. Maybe more common with multiple shapes on the map? I haven't figured out STR.

• There are validations on ISO_3166 a2/a3 codes to ensure they're two or three letters, but aside from that there are no controls: your shape need not actually correspond to your codes, A2 and A3 could be codes for completely different entities, etc.

  • codes can be searched in GZ autocomplete, other than that I believe they're currently unused

Both New GZ and Import GZs attempt to follow the same behavior for ensuring that any shapes created for GZs are:

• valid and
• do not cross the anti-meridian
  • My hot take as a person new to dealing with anti-meridian-crossing issues: "Wow, what a hot mess". Completely undocumented, with different behaviors both between libraries and within libraries. :p This was meant a little tongue-in-cheeck - TW at least has nice specs, maybe other librairies do too, maybe I just haven't looked hard enough. If there are things I'm just missing here please let me know.
  • the main issue as I see it is that functions processing anti-meridian-crossing shapes can treat a line between points on either side of the anti-meridian in different ways: some treat it as crossing the anti-meridian, some treat it as not crossing the anti-meridian but rather the meridian (the "long way around"). So even if your shape is reported valid?, other geo operations might operate on that shape in its opposite interpretation form and give "nonsense" results. Or your initial shape may be interpreted in the wrong way as invalid and then made valid in the wrong interpretation. So I've attempted to take the view that anti-meridian-crossing shapes just shouldn't exist in the db: some, maybe not all, are eventually going to explode or give the wrong result (though maybe not on operations we currently perform on them). I believe no GeographicArea shapes cross the anti-meridian(?).
  • Here was an instructive shape for me:
  • as drawn in leaflet:

as interpreted by leaflet after being normalized by our Gis::FACTORY, where lines between points now cross the meridian:

On that normalized shape, which is what we work with internally, GeographicItem.crosses_anti_meridian?, which checks for intersection with the anti-meridian line, returns true, as though the shape is the first picture, while rgeo valid? and postgis ST_IsValid return false as though the shape is the second picture (there's now a self-intersection at the right end which may be hard to make out). If you make_valid that shape it makes the second version valid, which is not what we want.

• The coordinate normalization performed by our Gis::FACTORY is significant here, with different normalization of the coordinates you get different results. This is where our use of ST_ShiftLongitude comes into play (or rather perhaps why it's needed?).
• I've attempted to document some of these issues with new specs/xspecs in the anti_meridian_spec file. Again, if I'm missing things or overly complicating things please do let me know.
  • the current behavior is to first check a new shape for crosses_anti_meridian? and if it does to use the new split_along_anti_meridian to cut it into pieces that don't cross the anti-meridian, and then to run make_valid on that shape. Since we're not checking valid before splitting along the anti-meridian, that can explode, and that's where I'm currently accepting failure: on (some) invalid shapes that cross the anti-meridian. Hopefully everything else is handled. More on this in the specs and commit notes.

[kleintom]

You can import the Ecoregions shapefile as found on the internet and it should complete with one error shape, but I'll have more to say on that file in the coming week. Maybe there's some way to provide a version of that shapefile that fixes that error shape and some other issues as well.

If anybody has shapefiles that they'd like me to test or that you test and that produce errors, I'd be more than happy to take a look at what's going on if you can provide me with the files or a link.

[mjy]

@jlpereira When you have time can you do another review of the JS side of things please?

[mjy]

@kleintom I talked with some folks here, where going to try and get a couple different shape-files to experiment with, some simpler, some complicated. More feedback in general coming soon.

[kleintom]

I feel like my understanding of anti-meridian behavior improved enough in the past couple days to make it worth adding some more clarification in the code. I'll try not to make anymore changes now.

It looks like the push is failing on a full db:migrate, which does https://github.com/SpeciesFileGroup/taxonworks/blob/development/db/migrate/20220927154908_add_cached_total_area_to_geographic_item.rb where GeoraphicItem::Point is referenced. Is the right fix for that to resurrect that class?

where going to try and get a couple different shape-files to experiment with, some simpler, some complicated. More feedback in general coming soon.

Sounds great, thanks!

[mjy]

migrate/20220927154908_add_cached_total_area_to_geographic_item.rb

This is a bit of a problem. We very strongly don't want to go back and alter migrations, however here I think we could try to add the logic "if the model is present, then run, if it is not, then skip". In this case it might be pretty safe. I'm not sure of another way to do this outside of injecting something into the migration wrapper, which feels very janky.

Hitting this issue also indicates that we likely need to rollup our individual migrations into one, and start fresh on new ones, noting this "fork" in the schema.

[mjy]

@kleintom Don't worry about pausing development, keep tacking on changes (date-line) if you wish.

[jlpereira]

Hello @kleintom

What fields are require to create a gazetteer? I'm trying to create one but I get error 500

ActiveRecord::StatementInvalid at /gazetteers.json
==================================================

PG::InternalError: ERROR:  Option string entry 'structure' lacks separator '='

> To access an interactive console with this error, point your browser to: /__better_errors

app/models/geographic_item.rb, line 1136
----------------------------------------

ruby
  1131     end
  1132   
  1133     def self.select_one(named_function)
  1134       # TODO use select_value instead of execute?
  1135       # This is faster than select(...)
> 1136       ActiveRecord::Base.connection.execute(
  1137         'SELECT ' +
  1138         named_function.to_sql
  1139       )
  1140       .to_a.first
  1141     end

App backtrace
-------------

 - app/models/geographic_item.rb:1136:in `select_one'
 - app/models/geographic_item.rb:361:in `make_valid_non_anti_meridian_crossing_shape'
 - app/models/gazetteer.rb:155:in `block in convert_geojson_to_rgeo'
 - app/models/gazetteer.rb:141:in `convert_geojson_to_rgeo'
 - app/models/gazetteer.rb:104:in `combine_shapes_to_rgeo'
 - app/models/gazetteer.rb:82:in `build_gi_from_shapes'
 - app/controllers/gazetteers_controller.rb:55:in `create'

parameters:

    "gazetteer": {
      "name": "Testing",
      "shapes": {"geojson":["{\"type\":\"Feature\",\"properties\":{},\"geometry\":{\"type\":\"Polygon\",\"coordinates\":[[[-69.023438,-28.214061],[-69.023438,-25.151913],[-62.958985,-25.151913],[-62.958985,-28.214061],[-69.023438,-28.214061]]]}}"],"wkt":[],"points":[],"ga_union":[],"gz_union":[]}},
      "projects":[13]
  }

[kleintom]

What fields are require to create a gazetteer? I'm trying to create one but I get error 500

Based on the error message I think/hope I fixed the issue, though it wasn't causing an error for me running in docker before or after the upgrade to pg16 (maybe you're running a pg with different config?).

Gazetteer should only require a shape and a (non-empty) name.

[jlpereira]

Thanks, It works!

[mjy]

@kleintom The blocker to us getting to Rails 7.2 has landed (https://github.com/rgeo/activerecord-postgis-adapter/pull/405#issuecomment-2455646003 ). We're still seeing issues on our test PR: https://github.com/SpeciesFileGroup/taxonworks/actions/runs/11672461015/job/32501118816, but I haven't poked at what might be the problem.

Given that we're likely a little away from landing this branch, I think we should target it landing on the 7.2 PR. Can you please update your branch to reflect this (should largely be pointing to a couple new gems)?

If there are spatial issues, which where the blockers in the adapter getting to 7.2, then your geo work/context might be the best place to discover these.

[kleintom]

Can you please update your branch to reflect this (should largely be pointing to a couple new gems)?

I copied @LocoDelAssembly's code to update to 7.2 and added what seems like a reasonable fix to get specs passing again (though I don't know exactly what changed in 7.2 to require it). It looks like ci won't run automatically anymore though since Gemfile.lock is going to be in conflict until the 7.2 pr lands.

[mjy]

@LocoDelAssembly if you have time can you look into the Gemfile/CI issue please.

@kleintom This might be the thread re the issue: https://github.com/rgeo/activerecord-postgis-adapter/issues/402#issuecomment-2371701919; and the patch: https://github.com/rgeo/activerecord-postgis-adapter/pull/417 ... maybe

[mjy]

@kleintom We saw that there is a migration to remove type from geographic_items (I think). Is it possible to leave that there until after this PR lands, or does it raise/get in the way? The issue is switching dev branches back and forth requires re-adding it.

[kleintom]

Is it possible to leave that there until after this PR lands

No problem, I just needed to make type nullable since we don't set it anymore.

[kleintom]

I was going to refactor code I added to normalize longitudes of new Points - to get this GI validation to pass: https://github.com/SpeciesFileGroup/taxonworks/blob/a804e9cef2e541bdf49af05f9b129c1aec7660f6/app/models/geographic_item/point.rb#L27-L31 - when I realized that the same issue arises with Multipoints and Points/Multipoints contained in GeometryCollections: rgeo normalizes longitudes for any shapes that have edges, but not for those mentioned above. Is point-longitude normalization something we want to pursue for Multipoints and Points/Multipoints in GeometryCollections? And/or do we want to keep it for Points? (Is the normalization mainly for UI display purposes?)

[mjy]

Reading this I had a momement of panic thinking maybe we had swtiched GeographicItem to Geometry type :( !, which would have been bad.

Multipoints and Points/Multipoints contained in GeometryCollections: rgeo normalizes longitudes for any shapes that have edges, but not for those mentioned above. Is point-longitude normalization something we want to pursue for Multipoints and Points/Multipoints in GeometryCollections?

I don't forsee needing either Multipoints or Collections thereof. Let's let users request it before we move on that front (I can't wrap my mind on what it would mean to assign such, but maybe, like in a Lot of fish lumped into a single barrel).

And/or do we want to keep it for Points? (Is the normalization mainly for UI display purposes?)

I don't recall why. It might be related to downstream checks at GBIF requiring such. Mapping might also have been an issue. If we can normalize on render (e.g. to map, or DwC, or...) that's a viable solution too, as long as we persist meaning unambiguosly.

[kleintom]

I'll have more to say, probably tomorrow, on the last new commit, but I wanted to get the first new commit out today since it fixes my previous attempt to restore GI type, which I think actually causes an exception on any read of an existing GA that references a GI with a non-null type (specs all passed since no spec references such a GA (apparently)).

[kleintom]

I finally got some notes written up (and figured out, I think) for the Ecoregions2017 shapefile - I put them here for now: https://gist.github.com/kleintom/c4c851de52380b0d08b6950e198aa8ba People just wanting a version of that shapefile that imports cleanly into TaxonWorks can just check the first couple paragraphs and skip everything else, which is more technical regarding the issues I encountered when importing the original shapefile (not all having to do with problems with the shapefile itself). There's a summary at the end so the longer account can be skipped. If there are any issues I missed please let me know.

I don't see a way around the first two RGeo-related issues (given our factory), that said I think those issues are both going to be quite rare, involving shapes crossing +/- 85.051127 latitude (two shapes involving Antarctica in the Ecoregions case) and lines intended to take the long way around the globe.

The third issue regarding what happens when imported shapes are made_valid has more wiggle room I think, maybe more reporting/warnings would be helpful there.

Lastly I'll point out the following postgis oddity (x)spec'ed in my last commit above:

select ST_Intersects(ST_GeographyFromText('LINESTRING (91 10, 0 0)'), ST_GeographyFromText('LINESTRING (180 89.0, 180 -89.0)'));

select ST_Intersects(ST_GeographyFromText('LINESTRING (91 0, 0 10)'), ST_GeographyFromText('LINESTRING (180 89.0, 180 -89.0)'));

The first returns false, as expected (for me at least): the line LINESTRING (91 10, 0 0) does not cross the anti-meridian, the second returns true: the line LINESTRING (91 0, 0 10) does cross the anti-meridian. The issue only occurs when one of the vertices is at exactly longitude 0, and the second vertex seems to have to be longitude-span > 90 away from 0, other than that I'm not seeing the pattern (feel free to shout if you know what's going on :-). Again, I think this is unlikely to be run into, but if it does become an issue I think the workaround outlined in the last commit message might work and not be too bad (or maybe it can get fixed in postgis if it is a bug).

I'm ready for more shapefile issues is anybody runs into some :-)

[mjy]

@kleintom Thanks very much. I just got my hands on some shapefiles and hope to hit the interfaces a little today. We also will collectively explore the UI during tomorrows help sessions, so a first wave of feedback should be coming shortly.

[LocoDelAssembly]

@kleintom can you please add import_gazetteers queue in exe/delayed_job and any other new queue this PR is adding?