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

pass custom mpi4py communicator #94

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

adematti
Copy link

@adematti adematti commented Oct 5, 2022

  • ability to provide, through the Python wrapper, the proper mpi4py communicator
  • drop use of MPI_ANY_TAG to avoid conflict with potential user communications

williamjameshandley added a commit to AdamOrmondroyd/PolyChordLite that referenced this pull request Jan 9, 2024
Copy link
Member

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

Hi @adematti, many thanks for putting this together. Apologies it has taken so long to get to review this. We're now in the process of trying to merge in a few PRs, and it would be great to include this one if you have time to look at this.

This change is actually really useful, and it looks like you've done some quite clever work to get this in place.

As submitted it was quite hard to review this PR, since your linter has made a lot of semantically irrelevant adjustments to the code, which might cause problems down the line as we try to merge in other changes. This was partly why I didn't have an opportunity to quickly review at the time. I've now reverted these.

Could you
a) check my resolutions given the latest changes to master
b) provide a bit of explanation for the semantically meaningful changes in setup.py? This is a file which has to accommodate several different setups (mac, linux etc), so should be changed with caution!

setup.py Outdated
Comment on lines 97 to 101
#cc_compiler = subprocess.check_output(["make", "print_CC"]).decode('utf-8').strip()
#os.environ"CC"] = cc_compiler

cxx_compiler = subprocess.check_output(["make", "print_CXX"]).decode('utf-8').strip()
os.environ["CXX"] = cxx_compiler
#cxx_compiler = subprocess.check_output(["make", "print_CXX"]).decode('utf-8').strip()
#os.environ["CXX"] = cxx_compiler
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these need to be commented out?

AdamOrmondroyd added a commit that referenced this pull request Jan 10, 2024
* fix errant backslash

* Adjusted the setup.py to not point to ccpforge

* Minor typo picked up in #94

* version bump

---------

Co-authored-by: Will Handley <wh260@cam.ac.uk>
Copy link
Member

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

@adematti I'm quite keen to get this merged, since I have a potential use-case for this myself.

Could you double check the changes I've made, and if you're happy I will merge them in.

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