Skip to content

Conversation

@chonlei
Copy link
Member

@chonlei chonlei commented Feb 8, 2021

Fix #1277

@chonlei chonlei requested a review from MichaelClerx February 8, 2021 14:46
r"""
A generalised version of the logit transformation for the model parameters,
which transform an interval or rectangular boundaries :math:`[a, b)` to
which transform an interval or rectangular boundaries :math:`(a, b)` to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transform -> transforms.

Is it a () interval then, not a half-open one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

() is an interval, yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you're saying the interval doesn't include a then? So it's an open rather than a half-open interval?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, need correcting that line too! 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an open () interval. I think now the docs is consistent...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D Are you sure? The rectangular boundaries docs say they define a half-open interval https://pints.readthedocs.io/en/stable/boundaries.html#pints.RectangularBoundaries

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... didn't realise that!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember the reason - why do we want to include the lower bound?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but maybe a separate ticket then? This can just fix the missing "s" and test :D :D :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1279.

@MichaelClerx
Copy link
Member

Tests are failing. I see Ben has a fix here: https://github.com/pints-team/pints/pull/1058/files

Maybe stick this fix in this PR too? Makes more sense here :D

@chonlei
Copy link
Member Author

chonlei commented Feb 8, 2021

@MichaelClerx
Copy link
Member

Michael did you mean stick this fix (https://github.com/pints-team/pints/pull/1058/files#diff-43f514bb09c1eb58423de94253a01bf25b3232f4324e4a9d8606ee6db00039b5R160) to here?

Yes please!

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #1278 (70e7233) into master (a6a1a39) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1278   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           84        84           
  Lines         8823      8823           
=========================================
  Hits          8823      8823           
Impacted Files Coverage Δ
pints/_transformation.py 100.00% <ø> (ø)

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 a6a1a39...9a95c24. Read the comment docs.

@chonlei chonlei merged commit 2f8d1b7 into master Feb 8, 2021
@chonlei chonlei deleted the i1277-transformation-docs-typo branch February 8, 2021 16:08
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.

Typo in RectangularBoundariesTransformation docs

3 participants