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

fix JRA55do runoff remapping bug #113

Closed
wants to merge 2 commits into from

Conversation

aekiss
Copy link
Contributor

@aekiss aekiss commented Aug 28, 2024

@aekiss
Copy link
Contributor Author

aekiss commented Aug 28, 2024

this will need to be cherry-picked into all other configs

@anton-seaice
Copy link
Contributor

@aekiss
Copy link
Contributor Author

aekiss commented Aug 29, 2024

Ah yes, good point! @ezhilsabareesh8 do you have a spare moment to do that?

Copy link
Contributor

@ezhilsabareesh8 ezhilsabareesh8 left a comment

Choose a reason for hiding this comment

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

Thanks, @aekiss! This looks good to me, and it's running without any issues. I can change the data_stream_xml_generation files to reflect these changes.

@dougiesquire
Copy link
Collaborator

Sorry to be a pain in the arse, but I don't think we want to get in the habit of making changes without updating the manifest. I understand they can make cherry-picking difficult, but they're an important part of the provenance.

Copy link
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

Also, it would be best to regenerate the datm.streams.xml and drof.streams.xml using the new generate_xml_*.py in om3-scripts so that they have the correct provenance metadata

@aekiss
Copy link
Contributor Author

aekiss commented Aug 29, 2024

Agreed @dougiesquire, I'd forgotten these files were script-generated so we should re-generate them (and while we're at it, fix COSIMA/om3-scripts#27 and COSIMA/om3-scripts#29).

Re. manifests, I think we should have one commit containing only the manifest changes, and another containing everything else. That way we can just cherry-pick the 2nd commit.

@dougiesquire
Copy link
Collaborator

Agreed @dougiesquire, I'd forgotten these files were script-generated so we should re-generate them (and while we're at it, fix COSIMA/om3-scripts#27 and COSIMA/om3-scripts#29).

Sorry, I was rushing and I merged COSIMA/om3-scripts#28 before seeing your new om3-scripts issues. I agree we should fix these both. @ezhilsabareesh8 could you please make these changes?

Re. manifests, I think we should have one commit containing only the manifest changes, and another containing everything else. That way we can just cherry-pick the 2nd commit.

Sounds good to me

@dougiesquire
Copy link
Collaborator

@aekiss, om3-scripts has now been updated to fix the above issues (thanks to @ezhilsabareesh8). I've updated the version of om3-scripts at /g/data/vk83/apps/om3-scripts, so you could use that to generate the new stream files.

@aekiss
Copy link
Contributor Author

aekiss commented Aug 29, 2024

Thanks @dougiesquire, I'm happy for somebody else to do that if you're waiting on it - I'm busy in a workshop tomorrow so might struggle to get to it.

@aekiss
Copy link
Contributor Author

aekiss commented Aug 30, 2024

thanks Dougie

@aekiss aekiss marked this pull request as draft September 16, 2024 05:28
@aekiss aekiss mentioned this pull request Sep 16, 2024
@aekiss
Copy link
Contributor Author

aekiss commented Sep 16, 2024

Closing - PR replaced by #123

@aekiss aekiss closed this Sep 16, 2024
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.

4 participants