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

Update the ptemcee dependency #137

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

yonatank93
Copy link
Contributor

The original ptemcee repo is not maintained. I changed the dependency to be my fork of the ptemcee repo. I have a minimal fix in my fork compared to the original repo, but it fixes some incompatibility, especially with newer numpy.

The original ptemcee repo is not maintained. I changed the dependency
to be my fork of the ptemcee repo. I have a minimal fix in my fork
compared to the original repo, but it fixes some incompatibility,
especially with newer numpy.
@yonatank93
Copy link
Contributor Author

@mjwen Just created a PR for the update for ptemcee dependency.

@mjwen
Copy link
Collaborator

mjwen commented Nov 16, 2023

@yonatank93 The CI test is failing. I took a look and seems there might be some specific syntax to add a git repo to extra_requires.

Can you try installing it locally and see whether we still have the error as in the CI test, and if yes, we need to fix it.

@mjwen mjwen self-requested a review November 16, 2023 01:56
We use Yonatan's fork of ptemcee as dependency, but that version of
package is not registered in pypi. So, we need to specify the link to
the repo (and the specific branch). But, apparently there is a
specific way how we can list such repo. So, this commit fixes this problem.
@yonatank93
Copy link
Contributor Author

@mjwen I was able to reproduce the issue. I think the problem is how I listed ptemcee in extra_requires. I think I found a way to fix it, and let's see if it works.

@mjwen
Copy link
Collaborator

mjwen commented Nov 16, 2023

@yonatank93 Yeah, it worked. Thanks!

One more request: can you delete the comments above extra_requires? Then we can merge it.

Since now we use Yonatan's fork of ptemcee, we don't need to limit
using numpy<1.2.0. The comment in this setup.py about this issue is
not necessary anymore.
@yonatank93
Copy link
Contributor Author

@mwen I just did it. Let me know if there are any changes that you want me to make. I also haven't worked on the uq test, as you suggested.

@mjwen mjwen merged commit 5fbee23 into openkim:master Nov 16, 2023
3 checks passed
@mjwen
Copy link
Collaborator

mjwen commented Nov 16, 2023

Let's wait until most of Amit's stuff gets merged and then we can think once more about how to adapt the UQ tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants