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 install requirements #52

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Conversation

AdamOrmondroyd
Copy link
Collaborator

The install_requires argument in setup.py is missing several requirements which are present in requirements.txt: anesthetic, scikit-learn, tqdm, and the platform-specific tensorflow.

torch was also missing from both setup.py and requirements.txt.

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8562358) 82.35% compared to head (a7b8ec6) 82.35%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #52   +/-   ##
=======================================
  Coverage   82.35%   82.35%           
=======================================
  Files           5        5           
  Lines         578      578           
=======================================
  Hits          476      476           
  Misses        102      102           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@htjb
Copy link
Owner

htjb commented Dec 12, 2023

Nice, thanks @AdamOrmondroyd! Clearly, I have been neglecting the requirements.txt. I think torch is only required for the tests and there is a requirement_tests.txt. Perhaps we could merge the two requirements files?

If you felt like it you could rewrite tests/clustered_distributions.py in tensorflow rather than torch? I stole this code from the link at the top of the file and all it does is generate samples/calculate probabilities for some standard multi-modal examples. Think we only need the log_prob functions for the two distributions but not 100% sure.

@htjb
Copy link
Owner

htjb commented Dec 12, 2023

Feel free to merge if you're not up for the clustered_distribution.py rewrite! Thanks @AdamOrmondroyd!

@AdamOrmondroyd
Copy link
Collaborator Author

Feel free to merge if you're not up for the clustered_distribution.py rewrite! Thanks @AdamOrmondroyd!

I'm reluctant to get too involved aha, I only stumbled on this from helping Dily

@htjb
Copy link
Owner

htjb commented Dec 12, 2023

@AdamOrmondroyd No problem hahaa it's not super important!

@AdamOrmondroyd
Copy link
Collaborator Author

I don't think I have permission to merge (and I'm partial to a green square)

@htjb
Copy link
Owner

htjb commented Dec 12, 2023

Hmm I think I added you as a collaborator so you can probably do it now...

@AdamOrmondroyd
Copy link
Collaborator Author

AdamOrmondroyd commented Dec 12, 2023

Hm I don't have a button
Edit: would help if I actually accepted the request lmao

@AdamOrmondroyd AdamOrmondroyd merged commit 5441db8 into htjb:master Dec 12, 2023
2 checks passed
@AdamOrmondroyd
Copy link
Collaborator Author

Might be worth pushing this to pypi so that pip install margarine slays?

@htjb
Copy link
Owner

htjb commented Dec 12, 2023

Yeah I'll sort it! Thanks @AdamOrmondroyd! 😄

@htjb
Copy link
Owner

htjb commented Dec 12, 2023

@AdamOrmondroyd Didn't bump the version number 😢

I'll push to master... bit cheeky but I'm allowing it 😆

@AdamOrmondroyd
Copy link
Collaborator Author

@htjb needs a test to check it's incremented... just nick the anesthetic one

@htjb
Copy link
Owner

htjb commented Dec 12, 2023

@AdamOrmondroyd Yeah I have one for globalemu thanks to @ThomasGesseyJones who may have also done the anesthetic one as well hahaa. Been on my margarine to-do list but haven't got to it yet.

@ThomasGesseyJones
Copy link
Collaborator

Hi @AdamOrmondroyd and @htjb ,

PR #53 adds the requested version control workflow (copied from the globalemu repository). Can one of you review the code before I merge?

For the record, I did not write the anesthetic version control workflow, at the time of writing the only workflow I have been involved with for that project was the automatic GitHub release workflow.

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