search

nanograv / enterprise

ENTERPRISE (Enhanced Numerical Toolbox Enabling a Robust PulsaR Inference SuitE) is a pulsar timing analysis code, aimed at noise analysis, gravitational-wave searches, and timing model analysis.
https://enterprise.readthedocs.io
MIT License
64 stars 65 forks source link

adding a hypercube transform function #366

Closed astrolamb closed 1 month ago

astrolamb commented 9 months ago

With @vhaasteren's ppf additions to parameter.py, I've written a short function to transform from a unit hypercube to the prior space for nested sampling

vhaasteren commented 9 months ago

Hi @astrolamb, the added hypercube transform looks fine to me. It just needs some added unit tests. I'm going to be a bit explicit about what else needs to be done. My explanation is probably more lengthy than what you need to do :)

However, the PR should probably be redone a bit. Ideally, when there is code you want to add upstream with a PR, you would start off with a fresh branch based on the dev branch. Then you add your code there, upload it to github under its own branch, and file a PR. Then your PR is clean and the diff is with respect to the dev branch. No conflicts.

Here you have started from a branch that diverges from Enterprise dev. You can either take your changes and transfer them to a fresh branch manually (easiest in your case), or use the git cherry-pick command to transfer your commits. You would do that by identifying your commit hashes using git log (you have 4 commits that are relevant), and then adding them to your fresh branch based off the dev brach like so:

git cherry-pick commit-hash

You do that for all relevant commits, in order. In your case, I would just copy over the function and file a new PR.

Anyway, extra complication is that your PR is actually based off of a PR of mine that is not yet merged, so you can't even file the PR yet. I could add your hypercube transform to my PR, or we can bother @AaronDJohnson to hurry up and review my PPF additions in #353, which is ready for review/merge. Or, yet another option is for you to file the PR against my own github ppf branch on github.com/vhaasteren/enterprise, which is the branch that is on queue for merge in Enterprise.

Lots of options for you. Anyway, for now your first order of business is to see if you can come up with a unit test. In tests/test_parameter.py you can see how unit tests are written. The parameter tests are executed with:

pytest tests/test_parameter.py

Those are also automatically checked by the github CI system, and your new code needs to be covered before it is allowed to be merged. If there are (too many) untested lines of code, the system will not allow you to merge.

astrolamb commented 1 month ago

I've improved this PR in PR #389. I will close this original one