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

adding a hypercube transform function #366

Closed
wants to merge 19 commits into from
Closed

Conversation

astrolamb
Copy link

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
Copy link
Member

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.

@vhaasteren vhaasteren self-requested a review November 15, 2023 11:10
@vhaasteren vhaasteren changed the base branch from master to dev November 15, 2023 11:11
@vhaasteren vhaasteren marked this pull request as draft November 15, 2023 11:11
@astrolamb astrolamb mentioned this pull request Jul 12, 2024
@astrolamb
Copy link
Author

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

@astrolamb astrolamb closed this Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants