COSIMA / esmgrids

Python representation of Earth System Model grids.
Apache License 2.0
4 stars 7 forks source link

Cice grid generation #6

Closed anton-seaice closed 7 months ago

anton-seaice commented 8 months ago

~Companion pull request to https://github.com/COSIMA/om3-scripts/pull/8~

EDIT: This PR also adds tests. See https://github.com/COSIMA/om3-utils/pull/5 for full scope of changes and review

anton-seaice commented 8 months ago

I tried to run the tests ... it looks like they rely on some test data which doesn't exist anymore.

I get a 404 error trying to download from this link:

http://s3-ap-southeast-2.amazonaws.com/dp-drop/esmgrids/test/test_data.tar.gz

anton-seaice commented 8 months ago

@aekiss & @ezhilsabareesh8 Can you review please?

anton-seaice commented 7 months ago

@aekiss & @ezhilsabareesh8 Can you review please?

Bump :)

micaeljtoliveira commented 7 months ago

@anton-seaice I suggest you drop the last three commits and rebase onto the latest main.

dougiesquire commented 7 months ago

@anton-seaice, I'm happy to do a review once you've brought in the relevant parts of https://github.com/COSIMA/om3-utils/pull/5 (i.e. the additional provenance metadata and the tests)

anton-seaice commented 7 months ago

@dougiesquire I think this is mostly done. Its a bit messy, which I will try and tidy up.

dougiesquire commented 7 months ago

@anton-seaice I'm not working today, but let me know when you're done making changes and I'll do a review first thing tomorrow

codecov[bot] commented 7 months ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

anton-seaice commented 7 months ago

@anton-seaice I'm not working today, but let me know when you're done making changes and I'll do a review first thing tomorrow

Thankyou! All done I think. Ive just been messing with test setup and comments :)

anton-seaice commented 7 months ago

Hi @dougiesquire I added your changes and fixed the tests :)

I'm still slightly unconvinced about dropping the git repo / hash stuff

dougiesquire commented 7 months ago

I'm still slightly unconvinced about dropping the git repo / hash stuff

~If we merge this, I can try sort out the automatic versioning and then we can address if you're still unconvinced~ UPDATED: actually I'll add here and see if you're happy

dougiesquire commented 7 months ago

@anton-seaice, I couldn't push directly to this branch so I've opened another PR here. Give it a try and see if you're now more comfortable dropping the git repo / hash stuff.

ADDED: info on the versioning can be found here: https://setuptools-scm.readthedocs.io/en/latest/usage/#default-versioning-scheme

anton-seaice commented 7 months ago

I realise the versioning is better / more accurate than git ... but its hard to get from this:

Created using esmgrid 0.1.dev108+g469ecfc.d20240416 to finding the package / code ??

dougiesquire commented 7 months ago

Created using esmgrid 0.1.dev108+g469ecfc.d20240416

Yeah, hopefully it would never look like that in production files, since the last part indicates uncommitted changes (which we can never keep track of). I would hope that all production files would be created with tagged releases ( so something like 0.2.0), but even unreleased commits will be kept track of (e.g. 0.2.0.dev<distance>+g<commit hash>).

anton-seaice commented 7 months ago

I think we should change “esmgrid” to the GitHub url ?

Get Outlook for iOShttps://aka.ms/o0ukef


From: Dougie Squire @.> Sent: Tuesday, April 16, 2024 9:13:40 PM To: COSIMA/esmgrids @.> Cc: Anton Steketee @.>; Mention @.> Subject: Re: [COSIMA/esmgrids] Cice grid generation (PR #6)

Created using esmgrid 0.1.dev108+g469ecfc.d20240416

Yeah, hopefully it would never look like that in production files, since the last part indicates uncommitted changes (which we can never keep track of). I would hope that all production files would be created with tagged releases ( so something like 0.2.0), but even unreleased commits will be kept track of (e.g. 0.2.0.dev+g).

— Reply to this email directly, view it on GitHubhttps://github.com/COSIMA/esmgrids/pull/6#issuecomment-2058838988, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AS4DACDUNSJOPV2OHI6V4STY5UBWJAVCNFSM6AAAAABE62WYPGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJYHAZTQOJYHA. You are receiving this because you were mentioned.Message ID: @.***>

micaeljtoliveira commented 7 months ago

Yeah, hopefully it would never look like that in production files, since the last part indicates uncommitted changes (which we can never keep track of). I would hope that all production files would be created with tagged releases ( so something like 0.2.0), but even unreleased commits will be kept track of (e.g. 0.2.0.dev+g).

One could add a check to stop the code if the file is being generated with uncommitted changes and issue a warning if it's not a tagged release.

dougiesquire commented 7 months ago

One could add a check to stop the code if the file is being generated with uncommitted changes and issue a warning if it's not a tagged release.

Sure, I'll do that now

dougiesquire commented 7 months ago

@anton-seaice, one more PR from me. Then IMO we're ready for a final review and merge

aekiss commented 7 months ago

Thanks @anton-seaice and everyone - great work!