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

DM-34589: Add a way to mark which dimensions populate others. #685

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

TallJimbo
Copy link
Member

@TallJimbo TallJimbo commented May 17, 2022

In the future, this will be used to make it easier to export dimension data without knowing the details of the dimension combinations that represent many-to-main joins (DM-34838).

There's no real gain to updating the dimensions configuration before that happens, but merging these changes early is useful because it will make software versions with just this change much better able to handle future data repositories that use them in their dimensions configuration, even if the associated functionality isn't available as a result.

Checklist

  • ran Jenkins local lsstsw
  • added a release note for user-visible changes to doc/changes

@TallJimbo TallJimbo changed the title DM-34838: Add a way to mark which dimensions populate others. DM-34589: Add a way to mark which dimensions populate others. May 17, 2022
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Patch coverage: 95.65% and no project coverage change.

Comparison is base (4927b47) 87.81% compared to head (2e8e241) 87.81%.

❗ Current head 2e8e241 differs from pull request most recent head 0bccc2b. Consider uploading reports for the commit 0bccc2b to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #685   +/-   ##
=======================================
  Coverage   87.81%   87.81%           
=======================================
  Files         274      274           
  Lines       36026    36049   +23     
  Branches     7535     7542    +7     
=======================================
+ Hits        31635    31657   +22     
  Misses       3224     3224           
- Partials     1167     1168    +1     
Files Changed Coverage Δ
python/lsst/daf/butler/core/dimensions/_config.py 84.81% <ø> (ø)
...ython/lsst/daf/butler/core/dimensions/_universe.py 88.48% <87.50%> (-0.06%) ⬇️
...ython/lsst/daf/butler/core/dimensions/_database.py 93.00% <100.00%> (+0.25%) ⬆️
...ython/lsst/daf/butler/core/dimensions/_elements.py 83.73% <100.00%> (+0.83%) ⬆️
tests/test_dimensions.py 97.07% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

I don't understand why this can't have any tests? Why can't we have a small test dimensions universe that has a couple of elements in where one populates the other. We have special test dimensions.yaml content in pipe_base and ctrl_mpexec. Why can't there be one call to get_elements_populated_by in one of the dimensions tests?

@TallJimbo
Copy link
Member Author

I can add some tests with a custom dimensions universe later in the week.

@TallJimbo
Copy link
Member Author

I think I'm changing my mind (a bit) again on how far to go on this ticket - I'm thinking now that we should update the default dimensions config to use populated_by, even if it doesn't do anything yet, because that will hopefully save repos created between now and some date in the future from needing a migration. But I still don't think we want to migrate any existing repos to use it.

So, since I think the rest of the migration changes are pretty close to merging over on DM-33942, let's hold this ticket up until that merges, then update the default dimensions config on this ticket after rebasing (and add a test based on that default config then).

@timj
Copy link
Member

timj commented Jul 21, 2023

#674 (DM-33942) was merged.

@TallJimbo TallJimbo force-pushed the tickets/DM-34589 branch 4 times, most recently from 8ef2d3e to 152a658 Compare July 28, 2023 17:28
doc/changes/DM-34589.misc.md Outdated Show resolved Hide resolved
In the future, this will be used to make it easier to export dimension
data without knowing the details of the dimension combinations that
represent many-to-main joins (DM-34838).

There's no real gain to updating the dimensions configuration before
that happens, but merging these changes early is useful because it
will make software versions with just this change much better able to
handle future data repositories that use them in their dimensions
configuration, even if the associated functionality isn't available as
a result.
@TallJimbo TallJimbo merged commit 490508e into main Aug 1, 2023
16 checks passed
@TallJimbo TallJimbo deleted the tickets/DM-34589 branch August 1, 2023 19:33
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.

2 participants