Skip to content

New method pad_missing to support aggregation of DSGs #718

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
Merged
davidhassell merged 7 commits into from
Feb 27, 2024
Merged

New method pad_missing to support aggregation of DSGs #718

davidhassell merged 7 commits into from
Feb 27, 2024

Conversation

@davidhassell
Copy link
Collaborator

@davidhassell davidhassell commented Feb 21, 2024

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.

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
Loading

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

sadielbartholomew commented Feb 27, 2024

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
@davidhassell davidhassell deleted the pad branch February 27, 2024 15:45
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