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

docs: draco1 vs draco2 comparison notebook #666

Merged
merged 17 commits into from
Aug 1, 2023

Conversation

peter-gy
Copy link
Collaborator

No description provided.

@peter-gy peter-gy marked this pull request as ready for review July 30, 2023 15:32
"| List ASP problem violations | ✅`drc1.Result.violations.keys` | ✅`drc2.get_violations` |\n",
"| Show how often a spec violates a preference | ✅`drc1.Result.violations` | ✅`drc2.count_preferences` |\n",
"| Generate ASP definitions from data | ✅ `drc1.data_to_asp` | ✅`drc2.schema_from_dataframe` |\n",
"| Conversion between spec formats | ✅ ASP ↔️Vega-Lite, CompassQL | ✓ ASP ↔️Nested dictionary, Vega-Lite |\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"| Conversion between spec formats | ✅ ASP ↔️Vega-Lite, CompassQL | ASP ↔️Nested dictionary, Vega-Lite |\n",
"| Conversion between spec formats | ✅ ASP ↔️Vega-Lite, CompassQL | ASP ↔️Nested dictionary, Vega-Lite |\n",

@@ -148,3 +151,17 @@ def answer_set_to_dict(answer_set: Iterable[Symbol], root=ROOT) -> Mapping:
collector[obj][prop] = collector[obj].get(prop, []) + [child]

return collect_children(root, collector)


def facts_to_dict(facts: Specification) -> Mapping:
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wonder whether we need this utility method or whether we better let people write it manually to make it clear what's happening. I would vote for not having a utility method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you mean. I added this method to have a one-liner conversion method, just like Draco 1 has. Given that we argue that Draco 2 has a nicer API, I figured it would come across as contradictory in the comparison notebook to have a seemingly more complex conversion flow in Draco 2 than what we have in Draco 1.

But since we would only benefit from this utility in the comparison notebook, we can probably live with this one-liner too:

answer_set_to_dict(next(run_clingo(facts)).answer_set)

I'll revert the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Short code isn't necessarily better. Composable APIs (imo) are best (with some caveats as it makes sense to have shortcuts for very common calls).

@peter-gy peter-gy force-pushed the docs/draco1-vs-draco2-notebook branch from 8d1e098 to 5aa8e53 Compare July 31, 2023 19:01
@domoritz domoritz merged commit 373962a into main Aug 1, 2023
9 checks passed
@domoritz domoritz deleted the docs/draco1-vs-draco2-notebook branch August 1, 2023 18:31
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

2 participants