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

package data integration #90

Merged
merged 3 commits into from Aug 21, 2023
Merged

package data integration #90

merged 3 commits into from Aug 21, 2023

Conversation

emanuel-schmid
Copy link
Contributor

@emanuel-schmid emanuel-schmid commented Aug 18, 2023

Changes proposed in this PR:

  • Move relevant content of data directory into the package directory

This PR fixes #86

PR Author Checklist

PR Reviewer Checklist

@emanuel-schmid
Copy link
Contributor Author

there is a test release on test.pypi.org:

pip install -i https://test.pypi.org/simple/ climada_petals==3.3.99

@emanuel-schmid
Copy link
Contributor Author

emanuel-schmid commented Aug 18, 2023

Most failing tests are because the climada_python develop branch has deviated.

There is one that fails because I didn't move one of the files, mistaking it for obsolete.
Fixed with 7fecd8f

@peanutfun
Copy link
Member

Most failing tests are because the climada_python develop branch has deviated.

These issues are fixed by #88 and #89, right? Then we should merge them first.

Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

I installed the test release and the data is copied correctly. Well done! I just have a few technical comments to the setup.py, but since it works, we do not need to dig too deeply into it.

I also ran a couple of tests on the installed version and most seem to work out. However, I found an issue: The tc_rainfield.py tries to import climada.util.tag (see https://github.com/CLIMADA-project/climada_petals/blob/develop/climada_petals/hazard/tc_rainfield.py#L34), but this has never existed in Climada. How did this escape our tests?
Edit: I just saw that #88 fixes this particular issue, so all good here

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@emanuel-schmid
Copy link
Contributor Author

| These issues are fixed by #88 and #89, right? Then we should merge them first.

Not if we want to make a patch release that just solves issue #86. This PR targets the main branch, #88 and #89 develop. Since Climada 4 is on the horizon we shouldn't spend too much time here. Just fix the pip release and then make a Conda release, I say.

@peanutfun
Copy link
Member

The changes to the tags in Core are not released yet, right? Then I am fine with merging this now. The tests should then pass when we test the released versions against each other 🤞

@peanutfun peanutfun merged commit 8fa1529 into main Aug 21, 2023
1 of 4 checks passed
@emanuel-schmid emanuel-schmid deleted the patch/package_data branch September 7, 2023 13:28
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.

climada-petals package does not contain the data directory
2 participants