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

Add editTopo.py tool #46

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Add editTopo.py tool #46

merged 2 commits into from
Dec 6, 2024

Conversation

micaeljtoliveira
Copy link
Contributor

No description provided.

@aekiss
Copy link
Contributor

aekiss commented Dec 5, 2024

@micaeljtoliveira
Copy link
Contributor Author

Yes, I think so.

editTopo.py Outdated Show resolved Hide resolved
@aekiss
Copy link
Contributor

aekiss commented Dec 5, 2024

If so, it might be good to include the commit hash or equivalently the URL https://github.com/COSIMA/topogtools/blob/6b4983127aa18dfdf1b62b2e18b581d82d4a64d4/editTopo.py in the git log to help with version control, since this script is now in 3 places and I'm not sure which of them should be thought of as definitive if we make improvements.

@micaeljtoliveira
Copy link
Contributor Author

@aekiss Good point! I've just added the url to the commit log.

I would suggest we treat the different versions in the following way:

  • the one here: used for OM3 and related models/configs.
  • the one in topogtools: used for OM2 and related models/configs

The same goes for all the other tools I think. Does this sound reasonable?

@aekiss
Copy link
Contributor

aekiss commented Dec 5, 2024

Yes that makes sense. It will be tricky to avoid divergence between the versions if we make changes, but it would be nice to keep them synchronised.

@micaeljtoliveira
Copy link
Contributor Author

My thinking was that we wouldn't need to update the tools used for OM2 very often, if at all, because AFAIK, there are no planned updates to the OM2 topography.

@aekiss
Copy link
Contributor

aekiss commented Dec 5, 2024

Fair point. So the version here should be though of as definitive/latest.

@aekiss
Copy link
Contributor

aekiss commented Dec 5, 2024

We could put a note to that effect in the topogtools README

@micaeljtoliveira
Copy link
Contributor Author

We could put a note to that effect in the topogtools README

Yes, I think that's a good idea.

I now realize I should have discussed this in more detail with you at some point, so that we would be on the same page. Sorry about that.

@aekiss
Copy link
Contributor

aekiss commented Dec 5, 2024

no problem!

@micaeljtoliveira
Copy link
Contributor Author

@aekiss I've now created an issue about the topogtools README.

Anything else needed here before merging this?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@aekiss
Copy link
Contributor

aekiss commented Dec 6, 2024

A couple more suggestions, but other than that it's ready to merge if the only thing you changed in the code was the GPL notice at the start (ie so I don't need to check the code).

@micaeljtoliveira
Copy link
Contributor Author

Yes, I've only added the copyright notice, everything else should be unchanged. Thanks for the review!

@micaeljtoliveira micaeljtoliveira merged commit a298d51 into master Dec 6, 2024
4 checks passed
@micaeljtoliveira micaeljtoliveira deleted the add_edittopo branch December 6, 2024 00:39
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