<div dir="ltr"><div>Markus,</div><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 21, 2023 at 8:26 AM Markus Demleitner <<a href="mailto:msdemlei@ari.uni-heidelberg.de">msdemlei@ari.uni-heidelberg.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Hi Mark,<br>
<br>
On Mon, Mar 20, 2023 at 02:22:10PM -0400, CresitelloDittmar, Mark wrote:<br>
> The UTypes and groupings look good for the most part.. a couple notes:<br>
> ** secondary comment that it'd be easier to read overall if the<br>
> PARAMref-s had the UTYPEs, but the PARAMs did not. But they did help me<br>
> find the match!.<br>
<br>
Well, I'll do whatever you (or preferably the spec) tell me :-).<br>
It's easy to remove the utypes from the PARAMrefs. Should I?<br></blockquote><div><br></div><div><br></div><div>As far as I know, there are no rules for which elements can/should have UTypes.</div><div>The spec doesn't mention/use PARAMrefs at all, but I like what you've got there.</div><div>It's very much like you have GROUP/PARAMref annotation for the flat PARAMs.</div><div>I do <b>not</b> think you should remove utypes from the PARAMrefs.</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">> o a few PARAMs with UTypes are not included in their respective GROUP.<br>
> - "spec:Spectrum.Target.Class"<br>
> - "spec:Spectrum.Char.FluxAxis.Ucd"<br>
> - "spec:Spectrum.Curation.Publisher"<br>
<br>
These are all easy, and I've moved them.<br></blockquote><div><br></div><div>Great!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
> - "spec:Spectrum.Data.SpectralAxis.order"<br>
<br>
This one is hard, because I can't (easily) put it in the general<br>
SDM-making code. How important is it to have it in the groups? Is<br>
this a validity issue?<br></blockquote><div><br></div><div>Well.. Section 8 of the spec says "the 'Data' element and the 'Point' elements </div><div>are implicitly represented by the table structure itself", but the VOTable has a GROUP</div><div>with utype="spec:Data" containing the data related elements.</div><div><br></div><div>I was looking at it from the implementation standpoint. If I can rely on your GROUPs to</div><div>define the objects/structure, then its much easier. With even just this one exception,</div><div>I'd have to interpret the GROUPs, and then the individual PARAM/FIELDs to look for</div><div>drop-outs and then figure out where they fit in the hierarcy.</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">> o two errors<br>
> - "spec:Spectrum.Target.Redshift" is in Target GROUP, but also in<br>
> Derived GROUP.<br>
> These are different (model-wise), and the Derived value would have<br>
> UType "spec:Spectrum.Derived.Redshift.Value"<br>
<br>
Ah. Yes, that makes sense. Dropped it from Derived.<br>
<br>
> - "spec:Spectrum.Length" has no value, but the DATA binary stream<br>
> definitely contains rows.<br>
<br>
As long as it's a NULL (rather than a 0 or some other concrete<br>
length): is that a validity issue? This is a bit of a special case<br>
in the echelle spectra. You see, normally these PARAMs are filled<br>
from SSA metadata in DaCHS, but for the orders I don't have the<br>
length in there, which is why this is NULL.<br>
<br>
I could do some special and extra code, but that's unattractive as<br>
long as nobody bothers to look there anyway (and I think clients<br>
bothering can just look into the data stream, somewhat more easily<br>
than I can, even).<br>
<br>
So, as long as I'm not lying and claim a bad length: is that really an error?<br>
<br></blockquote><div><br></div><div>OK.. Table 1 says the default is "(must be derived)", so I suppose a null value would indicate</div><div>that the reader must derive the value from the table. </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
> Other comments:<br>
> o most of the PARAMs have no value... so while they are there.. they<br>
> aren't terribly useful.<br>
<br>
Yeah; I'd suppress them if I then didn't have to tear down the<br>
references, too. Morale: If you're doing things like these groups,<br>
have a strong reason; references aren't free at all.<br>
<br>
> o UCD discrepancies (no comment on legality/correctness, just differences<br>
> from the spec.)<br>
> - "spec:Spectrum.Curation.PublisherDID": missing specified UCD<br>
<br>
Well, the UCD given in SpectrumDM is UCD-invalid. Another case for my<br>
point that we should stop requiring specific UCDs for columns and<br>
params in standards.<br>
<br>
I now went for meta.ref.ivoid from<br>
<<a href="https://wiki.ivoa.net/twiki/bin/view/IVOA/ObsCore-1_1-Erratum-1" rel="noreferrer" target="_blank">https://wiki.ivoa.net/twiki/bin/view/IVOA/ObsCore-1_1-Erratum-1</a>>;<br>
this could be a good opportunity to update this UCD in SpectrumDM,<br>
too (or, even better, make it explicit that the UCDs given in the<br>
example are good patterns but not normative).</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
> - "spec:Spectrum.Char.SpatialAxis.Coverage.Bounds.Extent":<br>
> spec="instr.fov" file="phys.angSize;instr.fov"<br>
<br>
Ah... instr.fov is a again invalid. See<br>
<<a href="https://wiki.ivoa.net/twiki/bin/view/IVOA/SSA-1_1-Err-2" rel="noreferrer" target="_blank">https://wiki.ivoa.net/twiki/bin/view/IVOA/SSA-1_1-Err-2</a>>. I'll not<br>
make this UCD-invalid -- perhaps update the spec?<br>
<br>
> - "spec:Spectrum.Length": missing UCD<br>
<br>
But that's the case in the spec, too? And putting meta.number here<br>
or so indeed doesn't seem helpful.<br>
<br>
> - "spec:Spectrum.Char.FluxAxis.Accuracy.StatError": UCD is missing a<br>
> bit at end. spec has "em.*" which would presumably be "em.wl" in this<br>
> dataset but file just has "em"<br>
<br>
The ";em" is fine *UCD-wise*. Whether it's helpful is another<br>
question, and given the "must be one of" on p. 15 of the 1.1<br>
document, I'm in violation of the current spec here (I frankly don't<br>
remember whether I did that on purpose all these years ago).<br></blockquote><div><br></div><div>The UCD discrepancies are more for the record than anything.</div><div>This is one place where the project scope can creep and cause delays.</div><div>We decided at the start that we were not trying to fix all the issues in the</div><div>current standard, but focus on adding the RFE elements.</div><div><br></div><div>I'll add an Issue to review the UCDs to the repo.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
It wouldn't be hard to fix this for this particular case, but in<br>
DaCHS I'd need to parameterise it so it won't lie when people publish<br>
spectra over frequency or energy, and that's an actual complication<br>
*to my adopters* that I frankly would only like to resign into if<br>
someone promises me they'll write code inspecting these UCDs one of<br>
these days (and then look for wl, freq, or energy). </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
So, does someone? If not, can we relax the "must" language?<br>
<br>
Thanks for the review,<br></blockquote><div><br></div><div>No problem! Thanks for the quick turn-around!.</div><div> </div><div> <br></div></div></div>