add MontePythonReader and MontePythonSamples classes #14

lukashergt · 2019-07-09T01:31:03Z

Created a new file read/montepythonreader.py containing the class
MontePythonReader which reads and processes MontePython chain files
using montepython.analyze.
- Extracts parameter names from MontePython's log.param file.
- Removes burn-in and non-markovian points from the data.
- Extracts tex names of the parameters.
- Extracts parameter limits from MontePython's log.param file.
The new class MontePythonSamples in sampler.py calls
MontePythonReader and turns the data into an MCMCSamples instance.

Changes to be committed:

modified: anesthetic/init.py
new file: anesthetic/read/montepythonreader.py
modified: anesthetic/samples.py

Checklist:

I have performed a self-review of my own code
My code is PEP8 compliant (flake8 anesthetic tests)
My code contains compliant docstrings (pydocstyle --convention=numpy anesthetic)
New and existing unit tests pass locally with my changes (python -m pytest)
I have added tests that prove my fix is effective or that my feature works

* Created a new file `read/montepythonreader.py` containing the class `MontePythonReader` which reads and processes MontePython chain files using `montepython.analyze`. - Extracts parameter names from MontePython's log.param file. - Removes burn-in and non-markovian points from the data. - Extracts tex names of the parameters. - Extracts parameter limits from MontePython's log.param file. * The new class `MontePythonSamples` in sampler.py calls `MontePythonReader` and turns the data into an `MCMCSamples` instance. Changes to be committed: * modified: anesthetic/__init__.py * new file: anesthetic/read/montepythonreader.py * modified: anesthetic/samples.py

codecov · 2019-07-09T01:43:05Z

Codecov Report

Merging #14 into master will increase coverage by 1.21%.
The diff coverage is 95.28%.

@@            Coverage Diff            @@
##           master     #14      +/-   ##
=========================================
+ Coverage   84.48%   85.7%   +1.21%     
=========================================
  Files          14      15       +1     
  Lines         986    1070      +84     
=========================================
+ Hits          833     917      +84     
  Misses        153     153

Impacted Files	Coverage Δ
anesthetic/read/chainreader.py	`66.66% <ø> (ø)`
anesthetic/read/multinestreader.py	`100% <100%> (ø)`
anesthetic/read/polychordreader.py	`100% <100%> (ø)`
anesthetic/read/getdistreader.py	`84.31% <100%> (ø)`
anesthetic/read/montepythonreader.py	`100% <100%> (ø)`
anesthetic/samples.py	`87.34% <100%> (-0.08%)`	⬇️
anesthetic/read/samplereader.py	`79.16% <79.16%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 440218a...79933a6. Read the comment docs.

williamjameshandley · 2019-07-09T07:44:40Z

I very much approve of a monte python reader.

Could you give me a bit more background on how a MontePythonSamples class differs from MCMCSamples? I would have thought that these were pretty much the same

lukashergt · 2019-07-09T10:35:39Z

I modelled MontePythonSamples to be very similar to MCMCSamples and potentially it could be incorporated in it, but then we'd need to decide on how to tell it to use the MontePythonReader instead of the GetDistReader.

williamjameshandley · 2019-07-09T10:43:28Z

My query is whether one needs an entire new class for MontePythonSamples. At the moment there are MCMCSamples, which covers getdist format (I believe that @Pablo-Lemos may have a cosmosis format, which if so should be offered as a PR when they have time), and NestedSamples, which covers MultiNest and PolyChord formatting.

I had envisaged that the role of the readers was to convert samples into one of these two classes to provide a unified 'inference interface'. NestedSamples are distinct from MCMCSamples, because they provide additional functionality such as evidence calculation and run-time replaying, but it's not clear to me what MontePython adds over GetDist in its samples which couldn't just be incorporated into the MCMCSamples class

lukashergt · 2019-07-09T10:49:32Z

The difference between MontePython and GetDist is similar to the difference between MultiNest and PolyChord I think. So, yes, we probably could have the MCMCSamples use the MontePythonReader.

I guess I am not sure how to make MCMCSamples differentiate between Montepython and GetDist.

williamjameshandley · 2019-07-09T10:54:45Z

Ah, I see. OK. You'll have to implement an MCMCReader, similar to a NestedReader, which handles recognising whether samples are in montepython format or getdist format. Have a look at anesthetic/read/nested.py, which organises the different cases for MultiNestReader and PolyChordReader

MCMCSamples would then use MCMCReader, rather than GetDistReader

* Created file samplereader.py which replaces nested.py and takes over its job of inferring from the root which sampler was used and calls the appropriate reader out of the following list: - MultiNestReader - PolyChordReader - GetDistReader - MontePythonReader * MontePythonReader was expanded by the properties `log_param_file`, `paramnames_file` and `log_file` which are used in `SampleReader` to determine whether the provided root is a MontePython root. * Deferred the proper calls to `montepython.analyze` to the `_init` method in `MontePythonReader` (previously already done in `__init__`) such that an instance can be created and its properties used even if the provided root actually is not a MontePython root. This allows the necessary checks in `SampleReader` to be done. * Removed `MontePythonSamples` as this is now handled by `MCMCSamples`. Changes to be committed: * modified: anesthetic/__init__.py * modified: anesthetic/read/montepythonreader.py * new file: anesthetic/read/samplereader.py * modified: anesthetic/samples.py

lukashergt · 2019-07-09T14:09:01Z

Ok, I've managed to merge MontePythonSamples into MCMCSamples. I have replaced the NestedReader by a more general SampleReader which distinguishes between MultiNest, PolyChord, GetDist, and MontePython.

What do you think?

williamjameshandley · 2019-07-09T14:23:22Z

This looks good. I like how you're using montepython's native chain reading. Makes me wonder whether a similar strategy should be applied for GetDist.

Are you able to rename montepythonreader.py to just montepython.py, or does that introduce a name conflict?

Ideally you should also add a couple of tests to confirm that the code works on a variety of machines. I assume that we want to keep montepython as an optional dependency, so it might be worth implementing an 'optional install' for the .travis system, as is done for fgivenx

williamjameshandley · 2019-07-09T14:24:18Z

How essential is it to use MontePython's native functionality (in case people just have chains, but don't want to install montepython)?

lukashergt · 2019-07-09T14:37:25Z

montepythonreader.py needs to be named that way else we have a conflict with the module (took me quite some time to figure that one out, rather silent error...)
How would I test for different machines? And how do you do an 'optional install'?
Instead of passing the folder as root you can pass the chain files as root which will lead to GetDist reading the files. However then montepython's removal of burn-in and non-markovian points is not performed and no limits are provided.

williamjameshandley · 2019-07-09T15:03:37Z

montepythonreader.py needs to be named that way else we have a conflict with the module (took me quite some time to figure that one out, rather silent error...)

I thought as much. Would it be worth suffixing the other files with 'reader' for consistency?

How would I test for different machines? And how do you do an 'optional install'?

Link to fgivenx above should give you the .travis.yml file modifications. Basically you have two different requirements files that it chooses to read from, one 'minimal' and the other with all optional dependencies installed. To do this you'd need to create the second requirements file, and extend the env section of .travis.yml a-la fgivenx.

Instead of passing the folder as root you can pass the chain files as root which will lead to GetDist reading the files. However then montepython's removal of burn-in and non-markovian points is not performed and no limits are provided.

It's worth remarking that the getdist ones should also potentially have burn-in removed, but anesthetic does not do this.

lukashergt · 2019-07-09T17:31:46Z

Would it be worth suffixing the other files with 'reader' for consistency?

I think that would be good. I actually generally think it is good when the module name reflects the class name therein.

It's worth remarking that the getdist ones should also potentially have burn-in removed, but anesthetic does not do this.

What do you mean? Add a comment in the docs of MCMCSamples that no burn-in was removed for getdist samples?

williamjameshandley · 2019-07-09T17:33:39Z

What do you mean? Add a comment in the docs of MCMCSamples that no burn-in was removed for getdist samples?

That would be for another PR. For now you could raise an issue that our system doesn't provide an interface for removing burn in.

lukashergt · 2019-07-09T22:30:09Z

montepython unfortunately is not pip installable, so I think we shouldn't put it in the requirements.txt file, right? How is this to work with the .travis.yml file then?

…with python3

… warn when MP updates cause trouble

…plaining of not being able to download 2.7 archive

.travis.yml

…thon as optional

…stallation issues with pypolychord which is not needed

…ly support OSX with python2

anesthetic/read/montepythonreader.py

tests/test_samples.py

…not installed * `MontePythonReader._init` raises an ImportError when montepython package is not installed. - `test_read_montepython` now xfails (instead of skipping) when montepython is not installed. Changes to be committed: * modified: anesthetic/read/montepythonreader.py * modified: tests/test_samples.py

lukashergt · 2019-07-10T10:59:25Z

That would be for another PR. For now you could raise an issue that our system doesn't provide an interface for removing burn in.

Then again, getdist only performs burn-in removal by discarding a certain fraction of the first steps. This would be easy enough to do with a pandas dataframe after the sample has been read with MCMCSamples...
Possibly all that is required here is warning about this in the documentation somewhere?

williamjameshandley · 2019-07-10T11:11:36Z

Then again, getdist only performs burn-in removal by discarding a certain fraction of the first steps. This would be easy enough to do with a pandas dataframe after the sample has been read with MCMCSamples... Possibly all that is required here is warning about this in the documentation somewhere?

Not if the chains come from multiple files which are then combined. If anesthetic labelled which chain the points had come from (not a bad idea anyway), then this would be easy enough to do, but it would be most easy if there were an additional keyword argument passed to MCMCSamples and the reader. Definitely worth raising as an issue (or two) to remind somebody to do this at some point.

williamjameshandley · 2019-07-10T11:32:38Z

tests/test_samples.py

+    mcmc = MCMCSamples(root='./tests/example_data/mp')
+    mcmc.plot_2d(['x0', 'x1', 'x2', 'x3'])
+    mcmc.plot_1d(['x0', 'x1', 'x2', 'x3'])
+


I like the use of this decorator to implement the conditional passing -- not something I've used before.

williamjameshandley · 2019-07-10T11:33:57Z

Many thanks @lukashergt . This kind of PR is essential for increasing the usage of this package

lukashergt added the enhancement New feature or request label Jul 9, 2019

lukashergt requested a review from williamjameshandley July 9, 2019 01:31

make import of montepython optional

74a728f

williamjameshandley added this to the 1.1 milestone Jul 9, 2019

Lukas Hergt added 2 commits July 9, 2019 14:48

remove nested.py as now handled by samplereader.py

1c53c00

Merge branch 'master' into montepython

742a883

rename read/*reader.py files for consistency

3929204

Lukas Hergt added 4 commits July 9, 2019 23:49

add installation of montepython to .travis.yml for tests

f7293e2

make .travis.yml only test montepython for python2 as not compatible …

90139c7

…with python3

download both montepython master and version 3.1.0 for .travis.yml to…

81747d3

… warn when MP updates cause trouble

changed language for OSX in .travis.yml back to generic as it was com…

c5631eb

…plaining of not being able to download 2.7 archive

lukashergt commented Jul 10, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

lukashergt commented Jul 10, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

add unit test for MontePython data

bbe4514

Lukas Hergt added 12 commits July 10, 2019 01:33

Merge branch 'master' into montepython

a7771b5

make pytest skip if montepython is not installed as we intend montepy…

5f00324

…thon as optional

add MontePython data to to use with unit tests

f6d10f3

install montepython without dependencies in .travis.yml to prevent in…

117d526

…stallation issues with pypolychord which is not needed

add missing semicolons in .travis.yml from previous commit

3c0cc23

make .travis.yml test for OSX test with montepython use python2

c71128d

2nd attempt to have .travis.yml test OSX with python2

0c4c8ab

3rd attempt to have .travis.yml test OSX with python2

feaa13d

4th attempt to have .travis.yml test OSX with python2

bc296a8

5th attempt to have.travis.yml test OSX with python2

81321f8

6th attempt to have .travis.yml test OSX with python2

12bb735

remove .travis.yml test of OSX with python2 as travis does not porper…

e22ef15

…ly support OSX with python2

williamjameshandley reviewed Jul 10, 2019

View reviewed changes

anesthetic/read/montepythonreader.py Show resolved Hide resolved

tests/test_samples.py Outdated Show resolved Hide resolved

williamjameshandley approved these changes Jul 10, 2019

View reviewed changes

williamjameshandley merged commit b53a707 into handley-lab:master Jul 10, 2019

lukashergt deleted the montepython branch July 10, 2019 15:47

williamjameshandley mentioned this pull request Jul 17, 2019

MCMCSamples cannot read nested sampling output files correctly. #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add MontePythonReader and MontePythonSamples classes #14

add MontePythonReader and MontePythonSamples classes #14

lukashergt commented Jul 9, 2019

codecov bot commented Jul 9, 2019 •

edited

Loading

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019 •

edited

Loading

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

lukashergt commented Jul 10, 2019 •

edited

Loading

williamjameshandley commented Jul 10, 2019

williamjameshandley Jul 10, 2019

williamjameshandley commented Jul 10, 2019

add MontePythonReader and MontePythonSamples classes #14

add MontePythonReader and MontePythonSamples classes #14

Conversation

lukashergt commented Jul 9, 2019

Checklist:

codecov bot commented Jul 9, 2019 • edited Loading

Codecov Report

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019 • edited Loading

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

williamjameshandley commented Jul 9, 2019

lukashergt commented Jul 9, 2019

lukashergt commented Jul 10, 2019 • edited Loading

williamjameshandley commented Jul 10, 2019

williamjameshandley Jul 10, 2019

Choose a reason for hiding this comment

williamjameshandley commented Jul 10, 2019

codecov bot commented Jul 9, 2019 •

edited

Loading

williamjameshandley commented Jul 9, 2019 •

edited

Loading

lukashergt commented Jul 10, 2019 •

edited

Loading