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

add inverse hyperbolic sin transform #360

Merged
merged 13 commits into from
Nov 3, 2023
Merged

add inverse hyperbolic sin transform #360

merged 13 commits into from
Nov 3, 2023

Conversation

pearsonca
Copy link
Contributor

Saw #297, and seemed easy enough to fix while I was looking at that area anyway.

@thomasp85
Copy link
Member

Would you like to update the PR to remove merge conflicts

@thomasp85 thomasp85 added this to the scales 1.3.0 milestone Nov 2, 2023
@pearsonca
Copy link
Contributor Author

Happy to - anything else I should address while in there?

@thomasp85
Copy link
Member

No, thanks. Better to keep the PRs lean and focused

@thomasp85
Copy link
Member

However, can I get you to add a bullet in NEWS about the new feature

@pearsonca
Copy link
Contributor Author

Can do re NEWS.

Question - I've written merge conflict resolution on probability_trans as moving the domain default setting to the signature. The existing resolution is setting it internally - what's the style preference here? I'd lean towards making it visible to the user via signature (and also allowing for some clever hacks with user defined q/p functions), but am also sympathetic to the-mathematical-definition-of-a-probability-is-...

@thomasp85
Copy link
Member

I think the domain should be fixed and set internally rather than as part of the construction

@pearsonca
Copy link
Contributor Author

I think the domain should be fixed and set internally rather than as part of the construction

Alright - I will adjust that (plus tweak test @olivroy mentions in comments).

I will also add the derivatives per #341

@thomasp85
Copy link
Member

Also, can you remove the derivative part - I rather we add that all together in one PR

@thomasp85 thomasp85 merged commit b885cf1 into r-lib:main Nov 3, 2023
11 of 12 checks passed
@thomasp85
Copy link
Member

Thank you 🙏

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.

4 participants