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

automatic minmax limits #80

Merged
merged 9 commits into from
May 22, 2020
Merged

Conversation

lukashergt
Copy link
Collaborator

@lukashergt lukashergt commented May 21, 2020

Automatically set all limits not specified by a .ranges file to their respective minimum and maximum values in the sample.

Fixes #36

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

@lukashergt lukashergt self-assigned this May 21, 2020
@lukashergt lukashergt added the enhancement New feature or request label May 21, 2020
@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #80 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   91.15%   91.19%   +0.03%     
==========================================
  Files          15       15              
  Lines        1323     1329       +6     
==========================================
+ Hits         1206     1212       +6     
  Misses        117      117              
Impacted Files Coverage Δ
anesthetic/samples.py 98.38% <100.00%> (+0.03%) ⬆️

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 6e953fd...1f3c9f0. Read the comment docs.

anesthetic/samples.py Outdated Show resolved Hide resolved
lukashergt added 4 commits May 21, 2020 14:16
`x0` and `x1` in the pc example data are both Gaussians with their limits set
to `None` in the .ranges file. I removed the limits on `x1` such that the
expected behaviour now (with auto minmax limits) should be:

* `x0` limits are set to (None, None) as specified in .ranges file
* `x1` limits will be set to (x1.min(), x1.max()) automatically
@williamjameshandley
Copy link
Collaborator

This looks great. I've had a quick look and it seems like it works well. Could you iterate the version number in the README to 1.2.6 to signify a change, and I'll formalise this as a new release.

@lukashergt
Copy link
Collaborator Author

I noticed one thing:
In the current setup, it doesn't assign limits to logL or weight. Would we prefer to have limits set on them, too? That might be better for cases where a PC run is ended prematurely and logL will have a sharp cutoff which could get washed out by the KDE without limits.

@williamjameshandley
Copy link
Collaborator

I think it makes sense to automate this too. It would be harder to communicate in the case that you don't want a hard limit, but you could always do that manually at the python level if you had to. You should probably also do this for the nlive column as well.

anesthetic/samples.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

This looks great. Please squash and merge!

@lukashergt lukashergt merged commit dac3a23 into handley-lab:master May 22, 2020
@lukashergt lukashergt deleted the minmax_limits branch August 10, 2022 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do we need .ranges files?
2 participants