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

Add a Way to Compare TimeDimensionSpecs While Ignoring the Grain #883

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented Nov 16, 2023

Description

This adds a key class for TimeDimensionSpec that can be used for comparing the specs while ignoring the grain. Useful for ambiguous group-by-item resolution where we want to match an ambiguous time dimension that the user has specified from a list of available time dimension specs.

@cla-bot cla-bot bot added the cla:yes label Nov 16, 2023
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@plypaul plypaul marked this pull request as ready for review November 16, 2023 21:56
Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@tlento tlento left a comment

Choose a reason for hiding this comment

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

Seems fine, might want a different approach, hard to say without seeing how (or if) it'll be used more broadly.

class TimeDimensionSpecField(Enum):
"""Fields of the time dimension spec.

The value corresponds to the name of the field in the dataclass. This should contain all fields, but implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

We might consider switching to dataclasses.fields or some other utility in the dataclasses module for this. Depends on what we're trying to do, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a little unclear how those would be used since those are dynamic, and I was looking for something specific I can reference in code.

Comment on lines +329 to +331
spec_field_values_for_comparison: List[
Union[str, Tuple[EntityReference, ...], TimeGranularity, Optional[DatePart]]
] = [self._source_spec.element_name, self._source_spec.entity_links]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth it to instead allow for a unique_name and type style equality comparison for all of these spec classes? I can see this being useful for more than just time dimension specs, although right now they're the only ones that have all of these extra properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tlento - mind elaborating? Would those be public properties?

Copy link
Contributor

Choose a reason for hiding this comment

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

They don't have to be public properties. I just mean doing a comparison based on the name/type of spec. Everything else we layer into specs as properties is transient information or something we need to propagate along, like annotations about aggregation state, aliases, stuff like that.

I did just realize that you're using the entire entity links, so my original suggestion doesn't work. That said, our comparison key is currently entity link path and name for linkable specs, and I suspect would be name for everything else. Will we need other properties? If we don't have specific things we'll need to add to that comparison then maybe we just add a method that does the simple thing.

@plypaul plypaul force-pushed the plypaul--58.5--consolidate-into-measure-input-spec branch 3 times, most recently from 3995915 to 274d987 Compare November 18, 2023 01:42
Base automatically changed from plypaul--58.5--consolidate-into-measure-input-spec to main November 18, 2023 01:51
@plypaul plypaul force-pushed the plypaul--58.6--time-dimension-spec-comparison branch from 602ab19 to f4c20db Compare November 18, 2023 03:29
@plypaul plypaul force-pushed the plypaul--58.6--time-dimension-spec-comparison branch from f4c20db to 86b41cc Compare November 20, 2023 18:29
@plypaul
Copy link
Contributor Author

plypaul commented Nov 20, 2023

@courtneyholcomb - I change the name of the module collections -> collection_helpers here, but we can change it to something else as well.

@plypaul plypaul merged commit 2128182 into main Nov 20, 2023
8 checks passed
@plypaul plypaul deleted the plypaul--58.6--time-dimension-spec-comparison branch November 20, 2023 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants