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-39605: Use butler.dimensions rather than deprecated butler.registry.dimensions #343

Merged
merged 10 commits into from Jun 10, 2023

Conversation

timj
Copy link
Member

@timj timj commented Jun 8, 2023

Checklist

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

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Patch coverage: 95.69% and project coverage change: +0.05 🎉

Comparison is base (2d6725e) 82.59% compared to head (bfed269) 82.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   82.59%   82.64%   +0.04%     
==========================================
  Files          66       66              
  Lines        7124     7133       +9     
  Branches     1387     1387              
==========================================
+ Hits         5884     5895      +11     
+ Misses        994      993       -1     
+ Partials      246      245       -1     
Impacted Files Coverage Δ
...on/lsst/pipe/base/_observation_dimension_packer.py 95.91% <ø> (ø)
python/lsst/pipe/base/configOverrides.py 87.77% <ø> (ø)
python/lsst/pipe/base/connectionTypes.py 87.09% <ø> (ø)
python/lsst/pipe/base/pipelineIR.py 85.35% <ø> (ø)
...ython/lsst/pipe/base/script/transfer_from_graph.py 12.50% <ø> (ø)
python/lsst/pipe/base/taskFactory.py 100.00% <ø> (ø)
...ython/lsst/pipe/base/tests/mocks/_data_id_match.py 94.91% <ø> (ø)
...ython/lsst/pipe/base/tests/mocks/_pipeline_task.py 34.23% <ø> (ø)
python/lsst/pipe/base/tests/no_dimensions.py 0.00% <0.00%> (ø)
python/lsst/pipe/base/tests/pipelineStepTester.py 0.00% <ø> (ø)
... and 26 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@timj timj requested a review from andy-slac June 9, 2023 23:00
@timj
Copy link
Member Author

timj commented Jun 9, 2023

@andy-slac (or even @TallJimbo) I'm not sure how you want to tackle this huge change or whether it's even worth while looking at it at all. In summary the changes are:

  • The intended change for butler.registry.dimensions -> butler.dimensions
  • Fixing lots of problems with links in docstrings (we have a habit of using DatasetType and Mapping but those won't be expanded properly).
  • Fixing all problems reported by pydocstyle and adding new GitHub action to check.
  • Changing all the type annotations to be the modern form (mypy still passes).

@timj
Copy link
Member Author

timj commented Jun 9, 2023

I also added a ruff configuration that will at least not swamp the user with suggestions. We aren't using ruff but it did throw up some interesting comments that could do with followup.

Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks good, a small bunch of minor comments.

@@ -197,10 +196,10 @@ def get(

def put(
self,
values: Union[Struct, List[Any], Any],
dataset: Union[OutputQuantizedConnection, List[DatasetRef], DatasetRef],
values: Struct | list[Any] | Any,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not Any make both Struct and list not useful at all? We should probably start using object instead of Any in places where we want some type checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I think it's using the annotation as a sort of documentation. Not sure whether to leave it as is or use Any and rely on the docstring.

python/lsst/pipe/base/connections.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/executionButlerBuilder.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/graph/graph.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/graph/graph.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipeTools.py Show resolved Hide resolved
python/lsst/pipe/base/pipeline.py Show resolved Hide resolved
python/lsst/pipe/base/pipeline.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipeline.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipelineIR.py Outdated Show resolved Hide resolved
@timj timj merged commit 5f77edf into main Jun 10, 2023
11 checks passed
@timj timj deleted the tickets/DM-39605 branch June 10, 2023 15:04
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