Conversation
gmantele
left a comment
gmantele
left a comment
There was a problem hiding this comment.
I have applied few changes (README, CI, SVG, ...) that were only in my PR #15 , but not here.
However, I have noticed few things that bother me. Can you, @jesusjuansalgado and Aitor comment about them (or fix them directly in the document, if needed)?
|
|
||
| Some of the parameters proposed in this standard are not described in | ||
| the ObsCore Data Model document , such as; min\_vis, max\_vis, | ||
| the ObsCore Data Model document , such as; min\_obs, max\_obs, |
There was a problem hiding this comment.
There should probably be a mention about the fact that the name of these columns has changed since the last version of this document. So, it probably worth to mention in the Change Log.
| called \textbf{VIS\_MIN}. This parameter would constrain the visibility | ||
| check to those time periods with at least the minimum visibility specified | ||
| in the parameter. The unit of \textbf{VIS\_MIN} parameters must be expressed | ||
| \item{\textbf{MIN\_OBS}\\A service \textbf{MAY} have a search parameter |
There was a problem hiding this comment.
min_obs seems to be the new name for two columns in the former version: min_viz (l.259 in old version) and viz_min (l.358 in old version). Is it normal?
There was a problem hiding this comment.
Yes, it is correct.
We wanted to clarify that we are no longer using the term visibility in any part of the document. That’s why we changed vis_min to min_obs.
| datatype="float", ucd="time.end" and unit="d" containing the end of the observability period.} | ||
|
|
||
| \item {Exactly one field \textbf{MUST} have a name="\textbf{t\_visibility}" | ||
| \item {Exactly one field \textbf{MUST} have a name="\textbf{t\_observability}" |
There was a problem hiding this comment.
This column name change should also be mentioned in the Change Log.
I guess the existing ObjVisSAP/ObjObsSAP service(s) MUST be updated too, isn't it?
|
|
||
| Some of the parameters proposed in this standard are not described in | ||
| the ObsCore Data Model document , such as; min\_vis, max\_vis, | ||
| the ObsCore Data Model document , such as; min\_obs, max\_obs, |
There was a problem hiding this comment.
max_obs is mentioned here, but is never described in this document, on the contrary to min_obs.
Is it normal?
There was a problem hiding this comment.
Thanks Gregory, I think all the points mentioned are relevant. I would let Aitor to edit directly the doc (I said to him that if he has some problem pushing from command line, small changes could be done directly in the portal). In case he still face problem pushing changes to this pull request, I could suppport
There was a problem hiding this comment.
Thanks for having check these so quickly.
About Aitor, his invite to join this repo has expired. That's why he was not able to push anything. I've sent a new invite. I hope it will solve his issue.
There was a problem hiding this comment.
If I remember correctly, we discussed time ago about to add max_obs as input. But then we decided to move it an optional output parameter.
I have added the description in the "Standard output fields" section.
max_obs description added to the optional output parameters.
Change log section added
gmantele
left a comment
gmantele
left a comment
There was a problem hiding this comment.
Seems OK to me now.
Feel free to merge whenever you want.
3 participants
No description provided.