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

JOSS Review -- ptemcee error while reproducing Coleman Bivariate Linear BMM models. #53

Open
gchure opened this issue Jan 11, 2024 · 17 comments
Assignees

Comments

@gchure
Copy link

gchure commented Jan 11, 2024

Hi! I'm reviewing your paper for JOSS and am working my way through assessing functionality. I cannot rerun the notebooks on my system following the install instructions on the GitHub repository.

Thus far, I've run into two issues:

  1. The sampler ptemcee is needed for Taweret to work, but is not included in the requirements.txt file.

  2. Training to find the posterior fails with the following traceback: coleman_BMM_traceback.txt. The error seems to be in ptemcee, which is a bit concerning to me as that package is no longer maintained.

Running the notebook through binder seems to solve the problem, but that is launching a version of Taweret that is not the one under review. This launches (and presumably executes) Taweret via danOSU/Taweret, as opposed to that hosted by bandframework/Taweret.

Screenshot 2024-01-10 at 16 11 37

I suspect this is on my end and not an issue with Taweret. Any suggestions at where to dig at this? I would like to be able to run through the example notebooks on the documentation so I can play around with the software and get to the more fun parts of the review.

@ominusliticus
Copy link
Collaborator

Hi! Sorry for the delay! We think this may be a problem with the bilby package and MacOS. For Linux, I don't seem to have this issue, and those of us that run MacOS, seem to be able to reproduce your error.

One thing to try to do is turn off multiprocessing by setting the thread count to one. Or using the npool keyword in place of the threads keyword in this screenshot

Screenshot from 2024-01-26 11-46-00

@ColmTalbot
Copy link

ColmTalbot commented Jan 29, 2024

Hi @gchure I'm one of the lead developers of Bilby and was asked about this by @ominusliticus via email. I managed to reproduce this issue and have a potential fix that I'll aim to get in the next release (see this issue). Although, I noticed that the recommended settings use Python=3.7 which we haven't supported for a few years (along with a large part of the scientific Python ecosystem) and so any fixes on our end won't be reflected with this Python version.

I'll add that I'm still not sure why this is an issue on MacOS, but not linux, although I have previously seen that changing the multiprocessing start_method can have meaningful changes.

In the shorter term, another fix is by using any of the other samplers supported by Bilby. With the default settings dynesty (rather than ptemcee) converged in less than a minute on two cores for me.

@ColmTalbot
Copy link

I think I'm correct above that this is related to the multiprocessing start method, I added

import multiprocessing
multiprocessing.set_start_method('fork')

to the top of the first cell and the sampling worked as expected.

@ominusliticus
Copy link
Collaborator

Thank you @ColmTalbot for taking a look at this!

@gchure
Copy link
Author

gchure commented Jan 29, 2024

Thanks @ominusliticus and @ColmTalbot, this is an interesting error and both of your suggestions fixed it on my system as well (running Python 3.10.9). I'll change the multiprocessing start method when evaluating the other notebooks for functionality.

@ominusliticus, I think this minimally deserves a page on the Taweret documentation that outlines i) what the problem is and ii) what the temporary workaround is to get this working for MacOS users. I would prefer it if you could add some toggle logic in the .train method that catches if the user is using MacOS and, if so, throws a warning and sets the multiprocessing start method context.

@ominusliticus
Copy link
Collaborator

@gchure I think this is a great suggestion, consider it done!

@gchure
Copy link
Author

gchure commented Jan 30, 2024

Thanks! It looks like the Binder link is still pointing to danOSU/Taweret. Can redirect these links to use bandframework/Taweret, which will ensure that the examples are using the correct Taweret version?

@asemposki
Copy link
Collaborator

asemposki commented Feb 2, 2024

I have changed the notebooks in main such that they now do not point to a Binder link but instead encourage the user to use GitHub Codespaces, by pointing them to the GitHub repo for Taweret (bandframework/Taweret). The reason for this is because Binder is taking a very long time to generate this repo and was not working for me when I attempted this a couple days ago, so I tried it on Codespaces and it works perfectly for me. I've also used Codespaces for summer schools, etc., and it seems to be a robust choice.

@asemposki
Copy link
Collaborator

Oh, I also note that the notebooks that run the SAMBA models for Bivariate Linear Mixing will error because I noticed I need to fix the SAMBA prior model dependence---I will get to this as soon as I can, and I think that is the end of the needed changes there.

@asemposki
Copy link
Collaborator

OK all SAMBA dependence has been integrated into this package, so I have eliminated the subpackages directory and all notebooks should run without needing SAMBA dependence.

@gchure
Copy link
Author

gchure commented Feb 7, 2024

Thanks for doing this! I just tried to run Linear_BMM_with_cdf_function_for_coleman_models on a Codespace, and am getting errors importing bilby (see screenshot). It would be great if things could be configured for users to immediately jump into a codespace and be able to run the examples without requiring more installation of components.

Screenshot 2024-02-07 at 11 36 44

@asemposki
Copy link
Collaborator

asemposki commented Feb 8, 2024

That's odd; I also tried running that notebook in the Codespace I just built (all I did was open a new Codespace and let it configure, and then chose a kernel and installed the typical Python extension), and it imports bilby for me. I'll look into this more and get back to you (I agree it would be nice if the entire thing loaded up and nothing else needed to be done by the user but run the notebook).

@asemposki
Copy link
Collaborator

I was never able to get the Codespaces issue to reproduce on my end, but adding an optional line to ! pip install bilby to the notebook can be done so that the user can uncomment this if they run into the same problem. For future development, it would definitely be cool to see a pre-set Codespace---I think we can add that to our list of middle- to long-term investigations. If this fix/workaround is OK with you, I will close this issue.

@asemposki asemposki self-assigned this May 7, 2024
@ominusliticus
Copy link
Collaborator

A follow-up on this: it seems what @gchure was suggesting is that a user wouldn't have to run any install commands, and that whatever packages are needed to run the notebooks already exists. This sounds a little like a container, which is thinking a bit ahead for the Taweret (and its parent, the BAND framework).

For now, we will add a section to the read me that instructs users how to run examples in the CodeSpaces

@asemposki
Copy link
Collaborator

Yes, exactly. I meant that we would definitely look into this for future work. I believe I wrote up the entirety of the Codespaces instructions to @ominusliticus once so I will work on adding that to the README as well.

@asemposki
Copy link
Collaborator

I have added instructions to the README.md file for running Codespaces and I have added the commented out line to pip install bilby in each notebook that uses it.

@asemposki
Copy link
Collaborator

If everyone is happy with these changes, I will close this issue.

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

No branches or pull requests

4 participants