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

Kpt extend format + bugfix in conversion sdor from old ERA5 #174

Merged
merged 5 commits into from
Nov 6, 2022

Conversation

sjboeing
Copy link
Contributor

@sjboeing sjboeing commented Nov 3, 2022

@leifdenby: this branch adds the seconds and date to the KPT format. It also solves an issue with the old units in ERA5: you will see this on quickly, it has to do with a change in units which we apply before we check the units.

@sjboeing sjboeing requested a review from leifdenby November 3, 2022 14:35
@sjboeing
Copy link
Contributor Author

sjboeing commented Nov 3, 2022

Not sure whether to bring this all into 0.1.0, or only the smaller bug fix. Let me know what is your preference.

@sjboeing sjboeing changed the title Kpt extend format Kpt extend format + bugfix in conversion sdor from old ERA5 Nov 3, 2022
@leifdenby
Copy link
Contributor

I think we should leave this for the next release. We can always make v0.1.1. I don't quite understand the changes you made (and reverted) for the sdor variable. So let's talk about this

@sjboeing
Copy link
Contributor Author

sjboeing commented Nov 6, 2022

Hi @leifdenby: thanks for having a look. Without the bugfix in line 372, it is not possible to produce KPT files, so I would like to change this one line for 0.1.0 if possible. The problem results from already changing the units via a dictionary (from "1" to "0-1", which I think is still required for some other variables) before changing them again.

Just ignore the reverted change (which is an alternative approach to fixing the issue).

The second (of the day) and date (as a number) are useful for the UM SCM, these fields are usually included in KPT files. With our date format they are duplicate information, but it makes it easier to read this information into fortran code. This change is less crucial, though I would be keen to include it quite soon.

@leifdenby
Copy link
Contributor

Hi @leifdenby: thanks for having a look. Without the bugfix in line 372, it is not possible to produce KPT files, so I would like to change this one line for 0.1.0 if possible. The problem results from already changing the units via a dictionary (from "1" to "0-1", which I think is still required for some other variables) before changing them again.

Just ignore the reverted change (which is an alternative approach to fixing the issue).

The second (of the day) and date (as a number) are useful for the UM SCM, these fields are usually included in KPT files. With our date format they are duplicate information, but it makes it easier to read this information into fortran code. This change is less crucial, though I would be keen to include it quite soon.

Ok, I'm just wary of changing more than one thing in a single pull-request, since this is both fixing a bug and making an addition. I'll write the changelog to indicate both changes and merge this before tagging and publishing v0.1.1

@sjboeing
Copy link
Contributor Author

sjboeing commented Nov 6, 2022

Thanks @leifdenby: sorry to bring this in last minute.

@leifdenby leifdenby merged commit 33873b8 into master Nov 6, 2022
@leifdenby
Copy link
Contributor

no problem :)

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.

2 participants