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

Allow DSG tractories with identical trajectory_id values to be aggregated #724

Merged
merged 10 commits into from
Feb 28, 2024
Merged

Allow DSG tractories with identical trajectory_id values to be aggregated #724

merged 10 commits into from
Feb 28, 2024

Conversation

davidhassell
Copy link
Collaborator

@davidhassell davidhassell commented Feb 26, 2024 •
edited
Loading

Fixes #723

Needs NCAS-CMS/cfdm#290 for the new test to work.

Full fix also needs #730

@davidhassell davidhassell added enhancement New feature or request aggregation Rerlating to metadata-based field and domain aggregation labels Feb 26, 2024
@davidhassell davidhassell added this to the 3.17.0 milestone Feb 26, 2024
@sadielbartholomew
Copy link
Member

sadielbartholomew commented Feb 28, 2024 •
edited
Loading

Hi David, I am in the middle of a meeting but afterwards I will complete my review. I noticed before that I messed up the one trivial conflict resolution for the change log, sorry (one entry was appearing at the top and bottom of the list and I didn't notice) - all that is needed is to remove one duplicated entry from another PR, so I will push up another commit to do that after the meeting.

sadielbartholomew
sadielbartholomew approved these changes Feb 28, 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.

Excellent. I've raised a pair of very minor comments but once you've considered those, please merge.

cf/aggregate.py Outdated Show resolved Hide resolved
cf/aggregate.py Outdated Show resolved Hide resolved
sadielbartholomew
sadielbartholomew approved these changes Feb 28, 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.

Just to confirm, all feedback addressed as far as I am concerned so please merge.

@davidhassell davidhassell merged commit 392ad50 into NCAS-CMS:main Feb 28, 2024
@davidhassell davidhassell deleted the dsg-aggregate branch February 28, 2024 19:08
@davidhassell davidhassell mentioned this pull request Feb 29, 2024
Merged
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.

Allow DSG tractories with identical trajectory_id values to be aggregated.
2 participants
@davidhassell @sadielbartholomew