Skip to content

Rounding counts for queries with count responses#220

Merged
costero-e merged 8 commits into
developfrom
develop_rounding-counts
Jul 1, 2025
Merged

Rounding counts for queries with count responses#220
costero-e merged 8 commits into
developfrom
develop_rounding-counts

Conversation

@costero-e

Copy link
Copy Markdown
Collaborator

This PR aims to solve the problem of not showing the exact detailed results for a query. The solution gives the possibility to have a value for rounded counts between two integers, like between 20-30 or a string, like >30 (more than 30). @jrambla to explain more shortly.

@mbaudis

mbaudis commented Apr 29, 2025

Copy link
Copy Markdown
Member

I understand the concept but not the inline description (referring to handovers)? Also the naming is a bit strange.

PrecisionCount ==> Precision IMO
rounded ==> imprecise

Something like imprecise had been discussed as concept before, going back to GA4GH Metadata Task Team.

So: Yes to work on this (I'll see later...).

@jrambla

jrambla commented Apr 29, 2025

Copy link
Copy Markdown
Contributor

As Michael points out, the descriptions in YAML are incorrect.
I've started a discussion to provide more details and proper descriptions here #221

This PR

* renames the `PrecisionCount` to `CountPrecision`, in line with the `countPrecision` parameter name
* removes the wrongly inserted description
* adds/modifies descriptions
@costero-e

Copy link
Copy Markdown
Collaborator Author

True, I forgot to remove the descriptions when recycling the Handovers entry to do the other entries. Thanks @mbaudis.

@mbaudis

mbaudis commented Apr 30, 2025

Copy link
Copy Markdown
Member

See changes in #223

@gsfk

gsfk commented May 1, 2025

Copy link
Copy Markdown
Collaborator

Regarding the enum ["exact", "rounded"] this ignores the possibility of imprecise counts that are not rounded, most notably when using differential privacy, I believe samply beacon already does this.

@costero-e costero-e requested review from gsfk and mbaudis June 12, 2025 14:27
type:
- integer
- string
minimum: 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be good to have examples.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I just created a couple from the discussion that @jrambla created. Check them out here b630915.

@gsfk gsfk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agreed to be a reviewer for this at our last dev meeting, but I see that I've already commented on it. This looks good, but I'll repeat my main issue, that "rounded" does not describe all inexact cases (with the main example being differential privacy).

I'm not sure what the best fix is:

  • change "rounded" to "inexact"
  • there are only two values, so use a bool instead of an enum (called either "exact" or "inexact" depending on your preference for bool direction)
  • add a third value to the enum to cover the case of inexact values that are not rounded. I don't have a value to suggest, in part because I can't think of one: something like "inexact" is not precise enough, since that applies to rounded values as well.

It would also be nice to have a general approach for numeric bins, but I think this is out of scope for this pr.

Descriptions still need to be cleaned up (including some json files where they are missing).

@mbaudis

mbaudis commented Jun 13, 2025

Copy link
Copy Markdown
Member

I'm with @gsfk here - "rounded" is just the wrong word; it doesn't have to be rounded - could be randomized, shuffled etc. (rounded is specific and if really rounded allows more backtracing). imprecise: true is best IMO (we've used "imprecise" before in GA4GH discussions, e.g. for geolocations).

@costero-e costero-e requested review from gsfk and mbaudis June 23, 2025 07:39
@costero-e

Copy link
Copy Markdown
Collaborator Author

Along with @jrambla, we have provided some descriptions and added the imprecise option for CountPrecision. Please, @gsfk and @mbaudis, review and we move forward. Thank you!

@mbaudis mbaudis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good with the changes; ready to merge.

But: I do not recall why we have 2 identical beaconCountResponseSection and beaconSummaryResponseSection? We'll have to review this w/ the aggregation/summary response edits.

@gsfk gsfk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Descriptions in beaconCountResponseSection{yaml, json} still need fixing (they're still a copypaste of the handovers description).

I'm not wild about the overlapping enums ("exact", "imprecise","rounded"), but I can live with them if everyone's really attached to keeping the term "rounded".

@costero-e costero-e requested a review from jrambla July 1, 2025 08:58
@jrambla

jrambla commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

@gsfk we have removed the unnecessary descriptions
Also, yes, the rare disease community usually use "rounding" as name and as operation.
We'll merge with these changes.
Thanks!

@costero-e costero-e added this to the 2.2.0 milestone Jul 1, 2025
@costero-e costero-e merged commit c110b82 into develop Jul 1, 2025
1 check passed
@costero-e costero-e deleted the develop_rounding-counts branch July 1, 2025 11:35
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.

4 participants