Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Doc Reference section #383

Closed
wants to merge 14 commits into from
Closed

Conversation

dr-jts
Copy link
Contributor

@dr-jts dr-jts commented Mar 12, 2019

Some more doc improvements. See commits for details.

Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
reference_operator.xml \
reference_output.xml \
reference_processing.xml \
reference_raster.xml \
reference_temporal.xml \
Copy link
Member

Choose a reason for hiding this comment

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

This is a historic thing. ST_ initially is SpatioTemporal, but by time to design the spec for temporal writers got tired and just said "ok let it be SpaTial". @estebanzimanyi you know the terminology about time, can you advise here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only functions under Temporal were ones dealing with Trajectories, so it seems better to give them a section by that name. That highlights the ability to work with trajectories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for the ST_ naming, maybe that reflected grand ambitions at one time. But AFAIK the specs haven't really fulfilled these dreams, so now it is just a (nice?) way to namespace spatial functions.

<refsynopsisdiv>
<funcsynopsis>
<funcprototype>
<funcdef>geometry <function>ST_Expand</function></funcdef>
Copy link
Member

Choose a reason for hiding this comment

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

but but but ST_Expand returns rectangular geometry and not a box, if fed with geometry. This minor distinction drove me nuts when I tried to normalize "what the function gets you from which input" graph, after which I decided that boxes are kind of rectangular areas. Also, ST_OrientedEnvelope - it's not a box?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, wasn't sure what was best to do with this. Ideally it would appear under both Geometry and Bounding Box. Too bad the name is overloaded, but there it is.

In the end I decided this was more of a bounding box kind of thing. But can change it back if that's better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And as for ST_OrientedEnvelope, I also find that name confusing. Perhaps it should be called ST_OrientedRectangle? But it's definitely not a box.

My take on naming practice is that the support of box* types is indicated by using the word "Box" in names. Other names like Rectangle and Envelope indicate that a geometry type is used. The use of the term Envelope at all is perhaps unfortunate, since in PG that function is provided by the box types. But that's what the Simple Features spec uses, so we should use it in a limited way.

doc/reference_bbox.xml Outdated Show resolved Hide resolved
<para>Returns the integer SRID of the
specified geometry column by searching through the GEOMETRY_COLUMNS table.
If the geometry column has not been properly added (e.g. with the
<xref linkend="AddGeometryColumn" /> function), this function will not work.</para>
Copy link
Member

Choose a reason for hiding this comment

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

AddGeometryColumn is not a good way to add columns since PostGIS 2?

Copy link
Contributor Author

@dr-jts dr-jts Mar 13, 2019

Choose a reason for hiding this comment

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

That's my understanding too. So this description should indicate this more strongly. But will need to research how to phrase this.

doc/reference_measure.xml Outdated Show resolved Hide resolved
<refnamediv>
<refname>ST_Accum</refname>

<refpurpose>An aggregate function to construct an array of geometries.</refpurpose>
Copy link
Member

Choose a reason for hiding this comment

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

ST_Accum is effectively array_agg. I understand it was added when array_agg wasn't available, but how about removing it now that there is no supported Postgres versions without array_agg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Can we get a third vote of +1?

doc/reference_sfcgal.xml Outdated Show resolved Hide resolved
@@ -122,19 +122,19 @@ XML_SOURCES = \
performance_tips.xml \
postgis.xml \
reference_accessor.xml \
reference_bbox.xml \
Copy link
Member

Choose a reason for hiding this comment

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

ST_Envelope, ST_OrientedEnvelope I'd expect nearby. Naming needs tweaking then - how about calling section with something like "rectangle functions" so box / geometry-with-rect-angles distinction is not there?

This may also consume operator section since these are defined on boxes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment. The Bounding Box section does not contain either "Envelope" function, so I think this is ok? When I reorg the Accessors and Processing sections we can think about where they should go.

My opinion is that they belong in different sections, even though they are unfortunately named similarly. One is just a simple accessor, but the OrientedEnvelope requires a full-blown spatial algorithm to compute. I think it should be called OrientedRectangle or something like that. It should not mention Envelope, because that implies an axis-oriented rectangle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the operator section should remain distinct, since it's long already. And operators are clearly different things to functions.

@estebanzimanyi
Copy link

estebanzimanyi commented Mar 13, 2019 via email

Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
Signed-off-by: Martin Davis <mtnclimb@gmail.com>
@dr-jts
Copy link
Contributor Author

dr-jts commented Mar 13, 2019

@estebanzimanyi @Komzpa Agreed, there's definitely no mandate to use a prefix other than ST_ right now.

I see now where the word Temporal comes from. However, "Temporal" has a much more general meaning in the database world. So it needs to be qualified in the PG context. Perhaps this section should be called Temporal Trajectory Functions? That would tie in with the OGC spec (which I wasn't aware of, and is nice to see). Although are there really any other kind of trajectories?

I note however that the PG functions do not seem to follow the OGC naming?

It might be nice to "namespace" the Trajectory functions. Maybe by using the prefix "ST_TrajectoryXXX"?

@dr-jts
Copy link
Contributor Author

dr-jts commented Mar 13, 2019

@Komzpa once again thanks for the detailed and thought-provoking review.

Can we make this PR just about reorganizing the reference section, and defer the issues of removing/improving docs to another issue? That will let this get committed and I can work on further improvements.

@dr-jts
Copy link
Contributor Author

dr-jts commented Mar 19, 2019

To summarize: apart from the reorganization which is the focus of this PR, for reference the other changes suggested are:

  • Possibly drop AddGeometryColumn doc, or make clear it is no longer needed?
  • Remove or deprecate ST_Accum
  • Possibly call section Temporal Trajectory Support ?

@dr-jts
Copy link
Contributor Author

dr-jts commented Mar 19, 2019

Is it possible to merge this PR? @strk @Komzpa

@strk strk closed this in 7db2b4c Mar 19, 2019
@Komzpa
Copy link
Member

Komzpa commented Mar 19, 2019

@dr-jts merged. a ticket linkr mentioned somewhere on the PR would be really helpful for composing a Closes: / References: section of merge svn commit message :)

@Komzpa
Copy link
Member

Komzpa commented Mar 19, 2019

Possibly drop AddGeometryColumn doc, or make clear it is no longer needed?

De-emphasize it. Go through docs and make sure we only mention it on its own page.

Remove or deprecate ST_Accum

I've raised it on mailing list.

Possibly call section Temporal Trajectory Support ?

My dream is to take @estebanzimanyi's work and make it part of PostGIS.

@estebanzimanyi
Copy link

estebanzimanyi commented Mar 19, 2019 via email

@dr-jts dr-jts deleted the doc-improve-ref branch March 21, 2019 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants