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

Cosmosis interface #33

Merged
merged 33 commits into from
Oct 18, 2018
Merged

Cosmosis interface #33

merged 33 commits into from
Oct 18, 2018

Conversation

joezuntz
Copy link
Collaborator

@joezuntz joezuntz commented Oct 17, 2018

This PR re-enables the interface of FireCrown to the CosmoSIS samplers.
Sampling with most of them will now work using firecrown sample file.yaml

Everything cosmosis-related is in the cosmosis subdirectory, and I've simplified it a bit since the previous version. If you don't have cosmosis installed the other stuff will still work.

Also I changed from sigma_8 to A_s in the examples, since the latter is the official parameter of DESC.

On a separate note - we should disable flake8 in the CI. It will put off too many potential contributors.

Closes #30

@beckermr
Copy link
Collaborator

beckermr commented Oct 17, 2018

flake8 is here to stay :p it is not that hard

look at all the beautiful code you wrote! :)

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

The changes look great! The code is well-commented and that is very helpful. Kudos!

Two things that need addressing:

  1. It appears the code will only sample cosmological parameters. This needs to be remedied I think.
  2. There are not any tests, unit/integration/otherwise.

I would add at least a few unit tests making sure that

  • parameters are passed between cosmosis and firecrown correctly
  • parameters get updated after each iteration of some sampler
  • all of the sampler parameters/options are getting to cosmosis

We are not going for 100% coverage. Just some tests of the basic functionality to make sure we are not shipping particularly egregious bugs. ;P

bin/firecrown Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
firecrown/cosmosis/interface.py Show resolved Hide resolved
firecrown/cosmosis/interface.py Outdated Show resolved Hide resolved
firecrown/cosmosis/run.py Show resolved Hide resolved
firecrown/cosmosis/run.py Outdated Show resolved Hide resolved
firecrown/cosmosis/run.py Outdated Show resolved Hide resolved
firecrown/cosmosis/run.py Outdated Show resolved Hide resolved
examples/cosmicshear.yaml Outdated Show resolved Hide resolved
@joezuntz
Copy link
Collaborator Author

I've added a set of unit tests - your numbers 1 & 3 are covered.
The second one is more of a regression test I think - we would have to build (or mock) a likelihood and run it. Unless you think the execute function should be broken up? Otherwise I'd prefer to leave that one out.

@joezuntz
Copy link
Collaborator Author

The CI seems to be using an outdated version of cosmosis-standalone.
But I can't see how it's installing it at all.

I've put something in the circleci to manually install it, but that seems like a bit of a hack - advice welcome.

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Some minor things. Tests look good otherwise. Still some comments left on the pr.

Concerning the tests I don’t get why running a sampler for a few steps serially and checking the output chains are called correctly requires MPI.

firecrown/cosmosis/interface.py Outdated Show resolved Hide resolved
firecrown/tests/test_cosmosis.py Outdated Show resolved Hide resolved
@beckermr
Copy link
Collaborator

About circleci. I am caching the python deps to make the tests faster. You should remove the extra install line and clear the cache in the circle interface. There is a drop down menu at the top right. Just click it and rerun without the cache.

We need to switch to a tagged version of cosmosis. This way pip will just install it if it is outdated.

@joezuntz
Copy link
Collaborator Author

I've added cosmosis with a specific version to the dev reqs. It sill needs some env vars, so I've put those in the circleci. (requiring the CC, CXX and FC values to be explicitly set saves an enormous amount of pain down the road when dealing with a multi-language plugin program)

The issue running a sampler has nothing to do with MPI. They can all be run serially. That's an entirely separate issue. I'll have a look at it.

@joezuntz
Copy link
Collaborator Author

Okay, there's a test of the sampling in there now too. Just the grid sampler, so that it's entirely predictable.

@joezuntz
Copy link
Collaborator Author

closed all but two conversations - let me know what you think on those.

@beckermr
Copy link
Collaborator

Looks great and thanks for all the work! I responded above.

firecrown/tests/test_cosmosis.py Show resolved Hide resolved
firecrown/tests/test_cosmosis.py Show resolved Hide resolved
@beckermr
Copy link
Collaborator

Just a few random extra lines in the docs and then it looks like we are good to go!

@joezuntz
Copy link
Collaborator Author

is it the ones between the parameter definitions that you don't like?
I like those, it makes it readable.
Or is it a different one?

@beckermr
Copy link
Collaborator

The doc strings. You missed a few.

@joezuntz
Copy link
Collaborator Author

think I got them

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I'll clean up the docs myself later if that is ok.

@joezuntz
Copy link
Collaborator Author

sure

@joezuntz joezuntz merged commit 436c4ba into master Oct 18, 2018
@joezuntz
Copy link
Collaborator Author

merged! Or did you mean you wanted to clean them up before merging? sorry if so

@beckermr beckermr deleted the cosmosis-interface branch October 18, 2018 14:48
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