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

Fix missing datetime "unit" and Ghrsst2ioda output "K" to "C" #834

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Fix missing datetime "unit" and Ghrsst2ioda output "K" to "C" #834

merged 3 commits into from
Jan 4, 2024

Conversation

apchoiCMD
Copy link
Collaborator

@apchoiCMD apchoiCMD commented Jan 4, 2024 •
edited
Loading

This PR fixes two bugs:

  1. Missing datetime "units"
  2. Ghrsst2ioda converter outputs sea surface temperature with unit "C"

All outputs are placed at each issue at #831 and #830

Close #831 , Close #830

ShastriPaturi
ShastriPaturi previously approved these changes Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@ShastriPaturi ShastriPaturi 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 to me.

AndrewEichmann-NOAA
AndrewEichmann-NOAA previously approved these changes Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@AndrewEichmann-NOAA AndrewEichmann-NOAA 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

guillaumevernieres
guillaumevernieres reviewed Jan 4, 2024
View reviewed changes
Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

We should probably entirely remove the option of outputting K and just assume everything is in C.

utils/obsproc/Ghrsst2Ioda.h Outdated Show resolved Hide resolved
guillaumevernieres
guillaumevernieres reviewed Jan 4, 2024
View reviewed changes
Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

that line is not needed anymore.

@apchoiCMD
Copy link
Collaborator Author

that line is not needed anymore.

ha! thanks for heading me up! Pushing again...

guillaumevernieres
guillaumevernieres approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

Thanks @apchoiCMD

@guillaumevernieres guillaumevernieres merged commit 45b902f into develop Jan 4, 2024
5 checks passed
@apchoiCMD apchoiCMD deleted the bugfix/unit-temp branch February 5, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guillaumevernieres guillaumevernieres guillaumevernieres approved these changes

@AndrewEichmann-NOAA AndrewEichmann-NOAA Awaiting requested review from AndrewEichmann-NOAA

@ShastriPaturi ShastriPaturi Awaiting requested review from ShastriPaturi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing datetime "units" The Ghrsst2Ioda converter should output C instead of K
4 participants
@apchoiCMD @guillaumevernieres @ShastriPaturi @AndrewEichmann-NOAA