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

enable optional MPI parallelism for desi_extract_spectra script #448

Merged
merged 1 commit into from Oct 25, 2017

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Oct 17, 2017

This PR adds a --mpi option to the desi_extract_spectra script to enable MPI parallelism at the command line level. Previously this was only available when running extractions within the context of the pipeline, which directly calls desispec.scripts.extract.main_mpi().

e.g.

srun -n 32 -c 2 --cpu_bind=cores desi_extract_spectra --mpi ...

It also fixes an inconsistency between the default wavelength step for the MPI vs. non-MPI versions.

@sbailey sbailey requested a review from tskisner October 17, 2017 20:16
@tskisner
Copy link
Member

This is a small change and obviously fine, but should we also go back and unify the mpi and non-mpi code into a single main()? This particular code was the very first one we did this to (adding mpi communicator to the entrypoint after moving the entrypoint into the package). Many of the other codes in desispec and desisim simply have one main with an optional communicator. However, we can also leave that for another day- no need to couple it to this small change.

@sbailey
Copy link
Contributor Author

sbailey commented Oct 25, 2017

Yes, I would like to merge those to different main functions, but purposefully didn't chase that in this PR. "another day..."

@sbailey sbailey merged commit 1bede4d into master Oct 25, 2017
@sbailey sbailey deleted the extract_mpi branch October 25, 2017 03:06
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.

None yet

2 participants