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

Adding examples for MIN/MAX interpretation. #38

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

msdemlei
Copy link
Contributor

This is intended to address bug #32.

@msdemlei
Copy link
Contributor Author

Ah... @pdowler – would you have a look at this?

Copy link
Member

@mbtaylor mbtaylor left a comment

Choose a reason for hiding this comment

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

"real" is not a VOTable datatype. The datatype attributes here should be "float" or "double".

@msdemlei
Copy link
Contributor Author

msdemlei commented Apr 12, 2023 via email

@mbtaylor
Copy link
Member

Content looks OK now but yes wait for @pdowler to comment.

On machinery for validating snippets of XML/VOTable, see ivoatexDoc Sec 3.11.4, which has an example very like this one. I'll admit it is a bit fiddly to do it for lots of small examples though.

@msdemlei
Copy link
Contributor Author

msdemlei commented Apr 12, 2023 via email

@pdowler
Copy link
Contributor

pdowler commented Apr 12, 2023

The description is technically correct in as far as it is consistent with the way xtype="circle" uses MAX for describing input params (for services via datalink service descriptor) aka minimum enclosing/spanning circle. The implications are a little different and it seems less useful in the context of a field (column full of circles) because a circle is a poor way to aggregate a set of circles and convey something useful about the set. There are several useful aggregates that tell the client about the circles in the column, but a single MAX interpretation is probably not one of them.... in an input PARAM, MAX isn't trying to aggregate.

So, the language here is consistent with the "xtype intepretation of MIN/MAX" idea we agreed on to resolve the usage for xtype="interval", but it also illustrates that maybe these interpretations that I'm supposed to write into WD-DALI-1.2 works for input PARAMs but not for FIELDs (where works means useful). I don't think an interval MIN/MAX example would expose that very well because 1D is, well, simple enough that it would usefully convey "coverage".

aside: the input param usage in service descriptors was driven by the use case of "provide useful info so client can do ops" (eg SODA) and we just tried to use existing VOTable features... it's not even the intended use of PARAM :-) Maybe using existing VOTable features was short-sighted and another solution supported by a VOTable enhancement would have been better

mbtaylor
mbtaylor previously approved these changes Apr 12, 2023
@Bonnarel
Copy link
Contributor

Bonnarel commented Apr 12, 2023 via email

@msdemlei
Copy link
Contributor Author

msdemlei commented Apr 13, 2023 via email

@msdemlei
Copy link
Contributor Author

msdemlei commented Apr 13, 2023 via email

@pdowler
Copy link
Contributor

pdowler commented Apr 13, 2023

I definitely do not think providing MIN/MAX for a FIELD where the column contains some kind of geometry is useful because those aggregate concepts don't really apply to that scenario like they do for scalar values.

As for PARAM, the generic usage in VOTable is that this is a constant value... a plain constant doesn't need a MIN/MAX to describe it. The usage in the service descriptor inputParam was already a bit of abuse, but I recall there was prior art for that usage. If a PARAM to describe an input is sanctioned VOTable usage then we can add some simple examples, but if it's something that other standards (DataLink, iirc SIA1, maybe others) hacked together then I'm less sure it belongs here.

@msdemlei
Copy link
Contributor Author

msdemlei commented Apr 14, 2023 via email

@pdowler
Copy link
Contributor

pdowler commented Apr 14, 2023

I am going to write up the DALI changes about MIN/MAX for some xtypes and create a PR today. I think once I've done that it will help us know what to put or not put into the VOTable doc. So let's hold off until I do that.

Will add link to that PR here when done.

@pdowler
Copy link
Contributor

pdowler commented Apr 17, 2023

OK, I have written as much text in DALI that I can and keep it general purpose. This is included in my current work tweaking the MOC xtype (because I forgot to check that it had not been merged):

ivoa-std/DALI#14

@msdemlei
Copy link
Contributor Author

msdemlei commented Apr 18, 2023 via email

@pdowler
Copy link
Contributor

pdowler commented Apr 18, 2023

Yes, I think changing the maximum circle example to an input PARAM would be better.

If you feel OK about having a forward reference to the usage in datalink service descriptors or SODA, you could explain the meaning of the "maximum circle" in that context. That's probably a better example and one based on actual use. It will say the same thing that WD-DALI says (in PR) but I think it's ok to repeat such explanations/examples when they illustrate use cases the spec supports.

If you want to make that max circle example more explicitly match the SODA use case, also change to PARAM name="circle" (because in SODA name="pos" datatype="char" arraysize="*" is something else).

@msdemlei
Copy link
Contributor Author

msdemlei commented Apr 19, 2023 via email

@pdowler
Copy link
Contributor

pdowler commented Apr 19, 2023

Looks good to me.

@tomdonaldson tomdonaldson added this to the v1.5 milestone May 11, 2023
@msdemlei
Copy link
Contributor Author

Can I bump this? @pdowler, can you perhaps do a formal review to save Tom the pain of force-merging? Thanks!

Copy link
Contributor

@tomdonaldson tomdonaldson left a comment

Choose a reason for hiding this comment

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

Thanks for the good discussion and apologies for the long review time. This seems good enough now and any further comments can happen in the Working Draft.

@tomdonaldson tomdonaldson merged commit 82fedf5 into ivoa-std:master Sep 1, 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.

5 participants