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

introduces a function called multiplicity to replace card[Ω](μ) #110

Merged
merged 7 commits into from
Oct 20, 2023

Conversation

hartig
Copy link
Contributor

@hartig hartig commented Jul 3, 2023

In the context of discussing the new Card(..) function introduced in PR #108, @afs writes:

card[Ω] evaluates to a function that gives the number of instances of μ.

We could change that "multiplicity" or something based on that word ("multiplicity" seems a bit long but may be necessary).

This PR here is a proposal to address this comment. In particular, this PR introduces a function called 'multiplicity' with a definition that can be cross-referenced, and (currently) one place where this function is used (namely, in the definition of Filter). The latter is to demonstrate how it would look like to use this functions instead of the notation card[Ω](μ). My proposal is to replace all occurrences of card[Ω](μ) in this way.


Preview | Diff

@hartig hartig changed the title introduces a function called multiplicity to replace card[Ω](μ) WIP: introduces a function called multiplicity to replace card[Ω](μ) Jul 3, 2023
@hartig
Copy link
Contributor Author

hartig commented Sep 28, 2023

@afs @kasei @Tpt @rubensworks This PR has been around for a while without attracting any discussion. Perhaps that is because I marked the PR as WIP? The reason why I did so is because the PR is not (yet) complete regarding the changes implied by the proposal that I am making here. I just wanted to first get your initial opinions on the proposal before I spend the time implementing the rest of the changes that would follow from the proposal. Therefore, can you please take a look at the preview of this PR and let me know what you think. The current changes are in particular in Section 18.3 and in the definition of the Filter operator (as an example to illustrate how the new multiplicity function would be used in the definitions of the algebra operators).

@Tpt
Copy link
Contributor

Tpt commented Sep 28, 2023

It looks great to me! Using "multiplicityΩ(μ)" confused me a bit during a first reading because I read the function name as just "Ω(μ)" and "multiplicity" as a sentence word. What about finding a way with typography (italics?) to make clear that "multiplicity" is here part of the function name and not part of the sentence?

@hartig
Copy link
Contributor Author

hartig commented Sep 28, 2023

It looks great to me! Using "multiplicityΩ(μ)" confused me a bit during a first reading because I read the function name as just "Ω(μ)" and "multiplicity" as a sentence word. What about finding a way with typography (italics?) to make clear that "multiplicity" is here part of the function name and not part of the sentence?

Good point. I have pushed commit a98dac6 which uses the feature of ReSpec to explicitly markup symbols in math formulas. Now the function name is rendered in italic font. Additionally, if you look at the HTML locally (it doesn't work in the preview), you can even click on every symbol within the definition to have other occurrences of the symbol highlighted within the definition.

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Should the <var> magic not be applied throughout, rather than being confined to the definition?

spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
@hartig
Copy link
Contributor Author

hartig commented Sep 28, 2023

Should the <var> magic not be applied throughout, rather than being confined to the definition?

Yes it should. However, I see that as something that is orthogonal to the proposal in this PR. Instead, I see it as something that we should do systematically across the whole document, as per issues #102 and #103.

@TallTed
Copy link
Member

TallTed commented Sep 28, 2023

I see that as something that is orthogonal to the proposal in this PR

Given that <var> has been applied to the definition, it's not being not applied anywhere, so it seems reasonable to apply it to all the text wrapped up in this PR, i.e., the discussion as well as the definition of the new multiplicity function.

If it must be done in separate PR(s), can you confirm that I applied it correctly to the complex expressions? I'm not sure I understand how things nest with it.

@hartig
Copy link
Contributor Author

hartig commented Sep 28, 2023

If it must be done in separate PR(s), can you confirm that I applied it correctly to the complex expressions? I'm not sure I understand how things nest with it.

I was just working on replies to your two change suggestions ;-) These replies should be visible now.

@afs
Copy link
Contributor

afs commented Oct 1, 2023

I agree that changing "cardinality" to "multiplicity" is an improvement.

It is difficult to have helpful styling here. Using the superscripts is one way but text does become small which may be an accessibility issue.

Choosing the presentation should not stop this PR because we can change the presentation later.

multiplicityLeftJoin(Ω1, Ω2, expr)(μ) =
    multiplicityFilter(expr, Join(Ω1, Ω2))(μ) + multiplicityDiff(Ω1, Ω2, expr)(μ)

There is more significant text in the the superscripts.

@hartig
Copy link
Contributor Author

hartig commented Oct 2, 2023

@afs

I agree that changing "cardinality" to "multiplicity" is an improvement.

That's a good start ;-)

It is difficult to have helpful styling here. Using the superscripts is one way but text does become small which may be an accessibility issue.

Yes, I can see this being a problem, in particular because, as you say, the text in the superscripts can be quite significant.

Choosing the presentation should not stop this PR because we can change the presentation later.

Given that my proposed change needs to be applied to quite a number of definitions (all in Section 18.5 SPARQL Algebra), and considering that switching from my currently proposed superscript-based presentation to some other presentation may not be easily done with a search-and-replace, I would prefer we agree on a presentation before I implement the rest of this PR.

So, here is an alternative to the superscript-based presentation: How's about we write multiplicity( μ | Ω ) to denote the multiplicity of μ in Ω? With this presentation, your example for LeftJoin would then be:

multiplicity( μ | LeftJoin(Ω1, Ω2, expr) ) =
multiplicity( μ | Filter(expr, Join(Ω1, Ω2)) ) + multiplicity( μ | Diff(Ω1, Ω2, expr) )

Is that better?

@afs
Copy link
Contributor

afs commented Oct 3, 2023

multiplicity( μ | LeftJoin(Ω1, Ω2, expr) ) =
multiplicity( μ | Filter(expr, Join(Ω1, Ω2)) ) + multiplicity( μ | Diff(Ω1, Ω2, expr) )

Is that better?

Yes.

I came back to this PR to suggest using a word multiplicity( μ in LeftJoin(Ω1, Ω2, expr) ). Your suggestion of | is equally good.

@hartig
Copy link
Contributor Author

hartig commented Oct 3, 2023

I have implemented the change to the presentation of the multiplicity function, using | as the separator (commit f1ae8c4).

I am planning to extend this PR to apply the proposal to all relevant parts of the document.

@Tpt
Copy link
Contributor

Tpt commented Oct 5, 2023

Thank you! | is much better. Nitpicking: I find it still a bit confusing because it is often a shortcut to "such that" like in ${ x | x \in \mu }$ and it's not the case here. in sounds slightly clearer to me. But it's nitpicking, | is very fine.

@hartig
Copy link
Contributor Author

hartig commented Oct 5, 2023

This PR is now ready for the actual review.

I have extended it by applying the proposal to all relevant parts of the document, and I have also switched to the correct terminology (cardinality -> multiplicity) in all the relevant places (see commit 7e6e6f9).

There are still a few mentions of "cardinality" but these really refer to the notion of cardinality of some set or some sequence, rather than to the multiplicity of an element within a multiset or sequence.

@hartig hartig changed the title WIP: introduces a function called multiplicity to replace card[Ω](μ) introduces a function called multiplicity to replace card[Ω](μ) Oct 5, 2023
@hartig hartig marked this pull request as ready for review October 5, 2023 20:02
hartig and others added 2 commits October 14, 2023 12:15
@TallTed
Copy link
Member

TallTed commented Oct 16, 2023

Now we have an example rendering issue
Screen Shot 2023-10-16 at 04 19 23 PM
In particular, note the collisions between t and ), and also (though to a lesser degree) between µ and ( and Ω and (.

The above is seen in Chrome Version 116.0.5845.187 (Official Build) (x86_64) on macOS Mojave version 10.14.6 (18G9323).

This would be resolved by my previous suggestion to add a space between all {}[]() and the non-whitespace characters they abut, or by (much more complicated) adding <i> tags to all {}[]() that abut a <var> or other character that is italicized for whatever reason.

@hartig
Copy link
Contributor Author

hartig commented Oct 17, 2023

Now we have an example rendering issue [...]

Strange. It looks perfectly fine in my browser (Firefox 118.0.2 on Ubuntu Linux):
image

@hartig
Copy link
Contributor Author

hartig commented Oct 17, 2023

... and it also looks fine in the Firefox on my phone:
Screenshot_20231017-113709

@hartig
Copy link
Contributor Author

hartig commented Oct 20, 2023

I think the only remaining point in this PR is the rendering issue that @TallTed has in his browser (whereas it looks perfectly fine on my end, in different browsers). Ted, given that this issue is not really related to the actual content of this PR, are you okay if I merge the PR?

@TallTed
Copy link
Member

TallTed commented Oct 20, 2023

For the record, the rendering issue I posted above is seen in Chrome Version 116.0.5845.187 (Official Build) (x86_64) on macOS Mojave version 10.14.6 (18G9323). (I'm also adding these details to the comment with that screencap.)

I'm fine with merging this PR as is, and treating the rendering issue as distinct.

@hartig
Copy link
Contributor Author

hartig commented Oct 20, 2023

Thanks @TallTed.

@hartig hartig merged commit 092d14f into main Oct 20, 2023
1 check passed
@hartig hartig deleted the Multiplicity branch October 20, 2023 19:16
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.

6 participants