ObsLocTAP review

[Markus Demleitner]

Dear DAL WG,

I've reviewed version 0.4 (20190215) of ObsLocTAP, and I'm pretty sure
it would be a good thing if we had such a standard.  I'd be happy to
assist anyone who'd like to use DaCHS to implement it as well as I can.
Kudos for having explicit use cases and showing how they are addressed
by the features.

But of course I have quite a few gripes on various details, so
here goes...


General points: 

(1) Whatever you use to manage your bibliography sometimes doesn't
quite get what's a first and what's a last name; while it's kinda cute
to see TAP referenced as Patrick, Guy and Doug 2010, I think it should
be fixed.  Note that you can pull a BibTeX of all the Recommendations
and a good part of the Notes from
https://volute.g-vo.org/svn/trunk/projects/ivoapub/ivoatex/docrepo.bib
(and of course my personal recommendation is to just use ivoatex for
authoring to start with, but that's probably not unbiased).

(2) I'm sure you've thought long and hard whether to include s_region and
there's a good reason not to have it.  But in case you'd just wanted to
see if someone misses it: I'd prefer if it was there and mandatory
whenever positions are published.  In particular for somewhat odder
kinds of instruments being able to give the observed region as a polygon
rather than a circle may be a big advantage, and I have a use case for
it below.  Plus, we now have engines that can deal with these geometries
reasonably efficiently.

(3) Here's another use case I'd propose: Global brokering.  There may be
unused observation time, perhaps because the seeing or weather doesn't
fit what was originally scheduled, perhaps because nobody had any use
for nights with too much moon; even ambitious amateur observatories
might put their observation time up for grabs.  They could then
advertise open (or opening) slots with an "area reasonably observable",
and researchers with "easy" targets might be able to make use of that
time if they only knew it was there.  What would be necessary to make
that work, I think, would be an "Available" (say) in execution_status
and the convention that the characterisation fields mean "reachable
area" then (which would the use case for s_region mentioned above).  Of
course, to make this really useful, some indication of the sort of
observations you could hope getting results for would be good.  In the
optical, that would probably be a limiting magnitude.  Cross-spectrum,
perhaps "limiting_flux" (in W/m^2, say) might work?

(4) Another use case I could see is "Common tooling for observers across
observatories": A tool can be written that lets observers monitor the
progress of their proposed observations that works for all observatories
that adopt ObsLocTAP.  Ideally, that would obviate custom,
per-observatory web forms, saving work for observatories and observers
alike.

(5) What's missing so far is the question of registering these services.
You *could* say "use TAPRegExt's dataModel"; as TAPRegExt's author,
however, I freely admit that dataModel was a mistake inspired by
Obscore, which is less typical than one might think in that it's a
singleton (i.e., there's only one obscore table per service).  True,
this property happens to hold for obsplan, too, but not for other DMs
(e.g., EPN-TAP), and it's better if there's a uniform way to locate such
data.  Also, "I implement data model X" is a property of a table or a
schema, not of a TAP service, so the TAP service's capability element
is also conceptually the wrong place to keep such information.

So, here's my take for a new section 4:

  Registering ObsLocTAP services

  ObsLocTAP services are registered as VODataService CatalogResources.
  They MUST have one or more auxiliary TAP capabilities pointing to the
  TAP service(s) at which the ivoa.obsplan table can be queried.  They
  furthermore MUST have a tableset, with the ivoa.obsplan table's utype
  set to ivo://ivoa.net/std/ObsLocTAP#table-1.0.

  An example for a registry record satisfying these constraints is given
  in appendix X.

  A RegTAP 1.1 query discovering the access URLs of all ObsLocTAP
  services served through TAP 1.x services would then be:

    SELECT ivoid, res_title, access_url
    FROM
      rr.resource
      NATURAL JOIN rr.capability
      NATURAL JOIN rr.interface
      NATURAL JOIN rr.res_table
    WHERE
      table_utype='ivo://ivoa.net/std/obsloctap#table-1.0'
      and standard_id='ivo://ivoa.net/std/tap#aux'
      and intf_role='std'
      and authenticated_only=0

  Clients probably want make sure the only query one access url per
  ivoid (this is in case multiple TAP capabilities are given for one
  resource).

I'm happy to provide a sample registry record, but I'd appreciate if
someone with a prototype service could give me the basic VOResource to
start with.


Individual points:

p. 7, "Making use of TAP, ..." to the end of the section -- I'd say the
document can be improved by dropping this text; the requirements you
cite haven't really been stated anyway (and I think they'd be hard to
derive from your use cases in that form), and I don't think anyone
should have to justify themselves if they're using TAP (on the
contrary).

p.7, sect 2.1 use case "Discovery of Observations" --  I'd say the
details of observation planning may not be too helpful for the purpose
of this standard.  On the other hand, use cases tend to interest me more
if they specify an actual task to be done (rather than describe a
desirable feature).  So, what about replacing this with:

  2.1 Avoiding unnecessary proposals for observation time
  
  An astronomer wants to propose an observation and can, before doing
  so, globally determine if a possibly suitable observation is already
  scheduled.  This might prevent costly duplicate observations.

  2.2 Support for weighting proposals

  The bodies that decide on the observation schedule can use the
  observation schedules of their own and other observatories to raise or
  lower priorities on a given proposal.  This might even take into
  account observation histories, giving demerits to possibly unnecessary
  repeated observations.

p.8, sect 2.2 -- I have to say I'm not terribly convinced SED creation
is a major use case for this particular protocol -- Obscore, SIAP or
even SSAP appear to be better suited here.

p.12, "Facility name should appear also with a real number."  -- there's
a "the" missing here I'd say, but more importantly: as facility_name is
a string, I'm pretty sure you want to write something else here.
Perhaps simply "The facility name should also be given"?

p.12, "If the astronomical target is still hidden, coordinates and
target should appear as null in the database so the ADQL queries do not
fail for the rest of the observations." -- the justification here seems
unncessary and misleading to me.  What values could be there that
could possibly make the "rest of the observations" "fail"?  I'd just
strike the entire bullet point; the rest of the text makes clear that
you can have target_name, s_ra, s_dec, and s_fov NULL in any
combination.

p. 13, "That would allow also the use of the ADQL geometrical
functions and constrains on the most relevant data model elements."
Actually, no, because TAP itself doesn't require any geometry support.
*If* you want to offer it (and I'd say that's a good idea), you could
say: "ObsLocTAP services MUST provide the following ADQL 2.1 features:
CIRCLE, POLYGON, CONTAINS." Or whatever.  If you don't want to depend on
ADQL 2.1, the features are defined in TAPRegExt 1.0 at this point.  But
I'd truly hope ADQL 2.1 will be REC by the time you go to RFC, so
referencing them there would be what I'd do.

p. 13, "In order to allow... with these column names" -- I feel the
introduction to 3.4 would profit from being shortened to just "The
following table gives the entries for the ivoa.obsplan table in
TAP_SCHEMA.columns."  The justification of TAP_SCHEMA is given nicely in
TAP itself, so I don't think it needs another motivation here.

p. 13, The table of columns: This should be updated to TAP 1.1 columns
(arraysize, and no adql:-types any more).  Also, there's no point
writing "not null (except...)" -- there's no way this can be encoded in
TAP_SCHEMA.  Just drop the constraint here and write, in the
descriptions of the columns in 3.1 something like "For execution status
values of Scheduled, Performed, and Aborted, t_min, t_max, and t_exptime
MUST NOT be NULL."  And by the way, I would just join the tables in 3.1
and 3.4 and then drop section 3.4.  It's always dangerous if you have
two tables that have a large overlap in content -- it's much too easy to
update one and forget the other.

Typos and such

p.9, "but, only, when the observations are already executed" -- there
shouldn't be any commas here

p.9, "so most of the metadata associated to the ObsDataSet could be
empty": s/could/will.  Later in that paragraph (next page): "could be
present only into" -> "could be present only within"

p.10 "ObsCore specification clearly" -> "the ObsCore specification
clearly"

p.10 "priory" -> "priori"

p.10 "(time, spectral, etc...), what implies" -> "(time, spectral,
spatial, polarisation).  This implies"

p.14 (and some other places) "ADQL sentence" -> "ADQL query".  If you
absolutely detest "query" here because it's a bit generic, make it
"expression" rather than "sentence".

         -- Markus