Skip to content

Conversation

@Qazalbash
Copy link

closes #993

@Qazalbash
Copy link
Author

@ColmTalbot

Copy link
Collaborator

@ColmTalbot ColmTalbot left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, just a few small comments.

It looks like one of the comments has been duplicated, sorry about that!

Also, congratulations on getting #1000!

@Qazalbash Qazalbash requested a review from ColmTalbot October 16, 2025 13:03
Copy link
Collaborator

@ColmTalbot ColmTalbot left a comment

Choose a reason for hiding this comment

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

It looks like it's generally rendering well, except for the issue with $S_n$

image

@Qazalbash
Copy link
Author

It looks like it's generally rendering well, except for the issue with $S_n$

image

Can you point to the issue in $S_n$?

@ColmTalbot
Copy link
Collaborator

I think it should be

$$S_n = \sum_{0}^{n - 1} x_i$$

This satisfies $S_0 = 0$ and $S_1 = x_0$.

@Qazalbash Qazalbash requested a review from ColmTalbot October 17, 2025 00:47
@ColmTalbot
Copy link
Collaborator

Thanks for continuing to work on this! I think I'm not being clear. For the definition of $S_n$, there is no distinction between $n < N - 1$ and $n = N - 1$, in all cases it is

$$ S_n = \sum_{0}^{n - 1} x_i \quad ; \quad n < N $$

Unless there's something else I'm missing.

@Qazalbash
Copy link
Author

It's because of the $S_0=0$ case. The current definition explains all; otherwise, there is a chance of confusion.

@ColmTalbot
Copy link
Collaborator

We currently have

$$ S_n := \begin{cases} 0 & n = N - 1 \\ \displaystyle\sum_{i=0}^{n-1}x_i & n < N - 1 \end{cases} $$

The top case here is not correct. Also, it doesn't mention $S_0$, which is also covered since $\sum_{0}^{0} x_i = 0$ trivially.

@Qazalbash Qazalbash requested a review from ColmTalbot October 17, 2025 18:01
Copy link
Collaborator

@ColmTalbot ColmTalbot 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 good to me now!

@ColmTalbot ColmTalbot enabled auto-merge October 27, 2025 14:05
@ColmTalbot ColmTalbot added the documentation A change to docstrings or doc pages label Nov 5, 2025
@mj-will mj-will added this to the 2.8.0 milestone Dec 11, 2025
Copy link
Collaborator

@mj-will mj-will left a comment

Choose a reason for hiding this comment

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

The equation looks good to me but I have questions on the examples.

auto-merge was automatically disabled December 22, 2025 18:40

Head branch was pushed to by a user without write access

@Qazalbash
Copy link
Author

@mj-will, sorry for the delay. You can see the fix in the recent commit.

@mj-will
Copy link
Collaborator

mj-will commented Jan 5, 2026

@mj-will, sorry for the delay. You can see the fix in the recent commit.

Thanks @Qazalbash, did you have a chance to check the first part of my comment here (for the N=1 case)?

@Qazalbash
Copy link
Author

Qazalbash commented Jan 5, 2026

@mj-will, I tried to accommodate in the current expressions, but it is hard to fit, because the $N=1$ should lie in the first condition, but it is forced into the latter one. A jugaad would be to write $N=1$ case separately.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation A change to docstrings or doc pages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

incorrect formula in bilby.core.prior.conditional.DirichletElement docstring

3 participants