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

RFE: order and limits #4

Merged
merged 3 commits into from
Jul 21, 2022
Merged

RFE: order and limits #4

merged 3 commits into from
Jul 21, 2022

Conversation

mcdittmar
Copy link
Collaborator

I had expected to make a separate branch for each item, but these effect the same diagram which made that a little more tricky, so I'm submitting them together.

This is an initial pass at satisfying Issues #2 and #3.

Things of note:

  • There is no PDF preview on the rfe branch
  • I simply made up the FITS column names, they seem reasonable to me.
  • I'm particularly uncertain about the UCDs
    • the reference in the UCD section points to some CDS file (which still exists), but is not an ivoa.net file. It may match one of those (I have not checked)
    • I looked at the referenced list to see if there was some way to distinguish 'order' from 'relorder' and the Upper/Lower limits from the StatErrHigh/Low.. but did not see anything that looked like a better fit.
  • In the schema file
    • I relocated the SED element, which let me minimize shifting things from one page to the next.

@lmichel
Copy link
Contributor

lmichel commented Jul 21, 2022

The preview generation is triggered when a commit occurs on the origin/wd-v1.2 branch.
We can not trigger it on multiple branch, because the reader wouldn’t know which branch the preview refer to.

The good way to proceed is to work on specific branches (e.g. rfe_order) and to operate PRs on wd-v1.2 (as if it was the main branch)
This also facilitate the review since the PR page show you all diffs.

@lmichel
Copy link
Contributor

lmichel commented Jul 21, 2022

The mist convenient way to build UCD is to query the UCD builder.
You are then pretty sure to get allowed UCDs

http://cdsweb.u-strasbg.fr/UCD/cgi-bin/descr2ucd

Lower limit: "detection lower limit" -> instr.saturation;stat.min
Upper limit: "detection upper limit" -> instr.saturation;stat.max

Spectral order "spectral diffraction order" -> stat.variance;instr.order (or just instr.order)
Spectral relative order "relative order" -> stat.value;instr.order(to be confirmed by the experts)

@mbtaylor
Copy link
Member

Once you've chosen suitable UCDs they should be checked for correctness. You can use Gregory Mantelet's Ucidy or the STILTS harness of it to do that.

It would be nice to include automatic checks for all the UCDs as part of the document build system. I tried this out for EPN-TAP by using a \ucd macro in the LaTeX wherever a checkable UCD was used, then in the Makefile test target pull all the UCDs out using grep and check them using STILTS.

You can see what that looked like at mbtaylor/EPNTAP@3cec868c2; if you like I could submit a PR to do something similar here once the UCDs look somewhat stable.

Copy link
Contributor

@lmichel lmichel left a comment

Choose a reason for hiding this comment

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

I suggest not to merge before the UCD issue is fixed

@mcdittmar
Copy link
Collaborator Author

Thanks Laurent,
The spec identifies a very specific file for the UCD vocabulary:
(The current list of valid UCDs is http://cdsweb.u-strasbg.fr/UCD/ucd1p-words.txt)

I assumed that list was rather old, where the UCD builder probably uses the latest approved list. So, I simply did a manual look through the list. It turns out:

  • a quick scan shows that list is approximately equal to V1.3 of the UCD list (2018) with a couple deltas. It does NOT include any deltas that went into V1.4
  • the results you got seem reasonable to me, but these will need review.

The mist convenient way to build UCD is to query the UCD builder. You are then pretty sure to get allowed UCDs

http://cdsweb.u-strasbg.fr/UCD/cgi-bin/descr2ucd

Lower limit: "detection lower limit" -> instr.saturation;stat.min Upper limit: "detection upper limit" -> instr.saturation;stat.max

Spectral order "spectral diffraction order" -> stat.variance;instr.order (or just instr.order) Spectral relative order "relative order" -> stat.value;instr.order(to be confirmed by the experts)

@mcdittmar
Copy link
Collaborator Author

I suggest not to merge before the UCD issue is fixed

I think I would prefer addressing the UCDs as a separate RFE/PR, especially if we are going to do more than just specify UCDs for these new element (ala Mark's suggestion of tagging them for automatic validation in the build). I'd like to open an Issue to build the action list for this.

FYI:

  • V1.1 had a pretty good review of the ucd list by the semantics group (noted in the change list)
  • There is an erratum seed from 2009 on this wiki page. It looks like the UCD itself was corrected for SpectrumV1.1, but the VOTable example was not. This should be on the action list for that issue.

@lmichel
Copy link
Contributor

lmichel commented Jul 21, 2022

2 points

  • The UCD list update is managed with VEPs (https://wiki.ivoa.net/twiki/bin/view/IVOA/VEPsForUCD) as for the vocabularies. We must keep this possibility in mind.
  • The UCD list only contains single words; you must keep in mind that UCDs words can be put together to build composed UCDs.

As you wish to address the UCD question in a separate issue, I can merge the PR.

@lmichel lmichel merged commit d0a65d4 into wd-v1.2 Jul 21, 2022
@mcdittmar
Copy link
Collaborator Author

That looks useful.. I see the test target is added to ivoatex/Makefile, this document has migrated toward, but not fully to using ivoatex. That shouldn't be a problem for the target, but was wondering where the \ucd{} macro spec lives... is that within ivoatex?

Once you've chosen suitable UCDs they should be checked for correctness. You can use Gregory Mantelet's Ucidy or the STILTS harness of it to do that.

It would be nice to include automatic checks for all the UCDs as part of the document build system. I tried this out for EPN-TAP by using a \ucd macro in the LaTeX wherever a checkable UCD was used, then in the Makefile test target pull all the UCDs out using grep and check them using STILTS.

You can see what that looked like at mbtaylor/EPNTAP@3cec868c2; if you like I could submit a PR to do something similar here once the UCDs look somewhat stable.

@mbtaylor
Copy link
Member

There's nothing magic about the \ucd macro, you can just define one yourself using \newcommand or whatever (and it can add some distinctive text formatting if you like, but it doesn't have to). The only requirement is that you can write some shell magic in the makefile to pull out the words in the LaTeX that are intended by the authors to be UCDs.

@mcdittmar
Copy link
Collaborator Author

Yes.. I understand this.
Since we are trying to make a minimal update to the spec, I did not want to change the referenced UCD list. Changing that to the 'latest and greatest' (or even an approved REC/EN) may invalidate UCDs within the document.

I'll open an Issue for the UCD topic shortly.

2 points

  • The UCD list update is managed with VEPs (https://wiki.ivoa.net/twiki/bin/view/IVOA/VEPsForUCD) as for the vocabularies. We must keep this possibility in mind.
  • The UCD list only contains single words; you must keep in mind that UCDs words can be put together to build composed UCDs.

As you wish to address the UCD question in a separate issue, I can merge the PR.

@mcdittmar mcdittmar mentioned this pull request Jul 21, 2022
@mcdittmar mcdittmar mentioned this pull request Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants