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.

Sign up for GitHub

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

Jump to bottom

New method pad_missing to support aggregation of DSGs #718

Merged
merged 7 commits into from
Feb 27, 2024
Merged

New method pad_missing to support aggregation of DSGs #718

merged 7 commits into from
Feb 27, 2024

Conversation

davidhassell
Copy link
Collaborator

Fixes #717

@davidhassell davidhassell added enhancement New feature or request aggregation Rerlating to metadata-based field and domain aggregation labels Feb 21, 2024
@davidhassell davidhassell added this to the 3.17.0 milestone Feb 21, 2024
sadielbartholomew
sadielbartholomew approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Implementation and tests are excellent. As well as for the VISION context this was made with in mind, I think these methods could be really useful.

Great stuff, please merge when ready.

cf/aggregate.py Outdated Show resolved Hide resolved
cf/data/data.py Show resolved Hide resolved
sadielbartholomew
sadielbartholomew approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Confirming a re-approval and that I have sanity checked the update resulting from #718. (I also went in to solve one trivial merge conflict whilst I was here so it is all ready to go.) Perfect, please merge.

@davidhassell
Copy link
Collaborator Author

davidhassell commented Feb 27, 2024 •
edited

Hi Sadie - I've added a new to_size keyword, because that's usually what you want to do ("pad it up to this size"), rather than working out how many points you need to pad to get it to the desired size.

54c72a5

@sadielbartholomew
Copy link
Member

Sure, that sounds useful. OK, I'll do a final review inclusive of the new commit. One moment...

sadielbartholomew
sadielbartholomew approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

New commit is all good (and a useful addition to the new method). Please merge (at last!).

@davidhassell davidhassell merged commit e5c13f3 into NCAS-CMS:main Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sadielbartholomew sadielbartholomew sadielbartholomew approved these changes

Assignees
No one assigned
Labels
aggregation Rerlating to metadata-based field and domain aggregation enhancement New feature or request
Projects
None yet
Milestone
3.16.1
Development

Successfully merging this pull request may close these issues.

New method pad_missing to support aggregation of DSGs
2 participants
@davidhassell @sadielbartholomew