Comments from Mark Taylor added #19
Conversation
gmantele
left a comment
gmantele
left a comment
There was a problem hiding this comment.
This looks OK to me. But let's wait for Mark review before merging as he is the comments author.
Note that the "../v1.3" is correct in the xmlns attribute, even though the VOTable version is 1.5.
mbtaylor
left a comment
mbtaylor
left a comment
There was a problem hiding this comment.
I made a validity fix to the example VOTable in section 5.
Other than that it looks good with one exception: the t_validity field is documented as mandatory, but it does not appear in the example response in section 5. Either downgrade t_validity to optional or add a suitable column to the VOTable.
ObjObsSAP.tex
Outdated
| confidence of the validity range, with one of the following allowed | ||
| \begin{itemize} | ||
| \item {A numeric field with name="\textbf{t\_validity}" \textbf{MUST} be present | ||
| with utype=" \textbf{Char.TimeAxis.Coverage.Time}" and unit="d", containing the date when the |
There was a problem hiding this comment.
For the second time, I am submitting the same review comment: Please remove the space after utype=".
| to check for object observability. The unit of \textbf{TIME} parameter must be | ||
| expressed in MJD. The format of the range list is the one defined by other | ||
| IVOA S*APs protocols like SSAP \citep{std:SSAP}, where the format of a | ||
| IVOA S*APs protocols like SSAP \citep{2012ivoa.spec.0210T}, where the format of a | ||
| range is defined by '$t_{min}\slash t_{max}$'. If the range is defined | ||
| as an open range without the lower value of the range like '$\slash t_{max}$', | ||
| as an open range without the lower value of the range like '$t_{max}$', | ||
| $t_{min}$ will be interpreted as now.\\ |
There was a problem hiding this comment.
I don't think the reference to the other Simple protocols is entirely correct or, at least, may be confusing to the reader.
For TIME:
- SSA does indicate its range-list values this way, with a
/, but uses ISO-8601. - SIA's
TIMEdoes use MJD, but indicates its range-lists with a space.
Maybe it's best to drop the reference entirely? Either place the reader looks, the examples would be incorrect.
There was a problem hiding this comment.
Fromt the SSA doc:
Allowable ISO8601 formats include the date (e.g., yyyy-mm-dd with the month
and day fields being optional, the minimum being yyyy), or the date-time (e.g., yyyy-mmddThh:mm:ss).
I will leave it as it is
There was a problem hiding this comment.
I would tend to say that we should not reference SSA. SIA and SSA should be merged at some point into DAP. That's why, referencing SSA (or SIA) does not seem like a good idea. Anyway, now, we have DALI in DAL which should be the basis of all DAL standards (SSA and SIA came after the birth of DALI). So, for new DAL standards, the recommendation is to follow as much as possible DALI. That what will happen to ConeSearch-1.1, for instance. So, in ObjObsSAP, I think that we should generally refer to DALI, when possible.
In this particular case, DALI-1.1 and 1.2 explicitly recommend to use an ISO8601 (like) format. There is an xtype for range or interval, but none applies to timestamps (but instead to numeric values). Maybe that should be something to speak about with Pat.
There was a problem hiding this comment.
Fromt the SSA doc: Allowable ISO8601 formats include the date (e.g., yyyy-mm-dd with the month and day fields being optional, the minimum being yyyy), or the date-time (e.g., yyyy-mmddThh:mm:ss).
I will leave it as it is
I agree with Grégory's point overall that DALI should be followed, so my correction is somewhat moot.
The point of my original comment was to point out that these are the ways of providing a time range in ObjObsSAP and the above mentioned protocols:
SSA: 2025-01-01/2025-12-30 <- ISO(SLASH)ISO
SIA: 59522 59532 <- MJD(SPACE)MJD
ObjObsSAP: 59522/59532 <- MJD(SLASH)MJD
So the statement I suggested correcting:
The format of the range list is the one defined by other IVOA S*APs protocols like SSAP [], where the format of a range is defined by 'tmin/tmax'
is because ObjObsSAP does not specify its time-ranges like the other S*AP protocols (since they don't agree with each other anyway)
There was a problem hiding this comment.
Since RESPONSEFORMAT is implied as being supported in Sec 4.1, perhaps it should be added to the Non-compulsory Parameters section?
Co-authored-by: Joshua Fraustro <36318163+jwfraustro@users.noreply.github.com>
Co-authored-by: Joshua Fraustro <36318163+jwfraustro@users.noreply.github.com>
aibaiba
left a comment
aibaiba
left a comment
There was a problem hiding this comment.
fix issues from esabol and jwfraustro
No description provided.