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

New tutorial on importance sampling with X-PSI #473

Merged
merged 19 commits into from
Dec 13, 2024

Conversation

yveskini
Copy link
Contributor

@yveskini yveskini commented Dec 5, 2024

No description provided.

@yveskini yveskini linked an issue Dec 5, 2024 that may be closed by this pull request
@thjsal
Copy link
Contributor

thjsal commented Dec 6, 2024

I think we should probably move this tutorial preferably to the docs (so no not to keep it in the examples folder as now), so that it will be visible also from the web pages. I can move this to there. I am also now checking if everything works and looks fine.

@thjsal thjsal self-assigned this Dec 6, 2024
Copy link
Member

@drannawatts drannawatts left a comment

Choose a reason for hiding this comment

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

Reviewing just the tutorial notebook:

  1. The line "For the new model, we changed the spin frequency from 314.0 Hz (which was used to generate the data) to 310.0 Hz (this is technically not the correct model, it's just an example)" - the final bit reads a bit strangely to me since if we changed frequency we'd also change the pulse profile. Could we perhaps say instead something like:

"[Note: if we actually picked a different frequency in the model (perhaps because the true spin is uncertain), we would technically have to re-generate the pulse profile using that frequency as well. Here we do not do that, which is equivalent to assuming that the data set is short enough that the change in frequency has not affected the pulse profile. We've done this just because it makes a nice and fast example to show! "

Feel free to rephrase as you like!

  1. "new posteriors looks like" -> "new posteriors look like"

  2. Not sure what you mean by "(look density method)"? Also there needs to be a full stop at the end of that paragraph.

Otherwise looks good to me!

@thjsal
Copy link
Contributor

thjsal commented Dec 6, 2024

I merged the main main branch to this branch, and the importance sampling notebook seems to be not working anymore. Need to fix some prefixes at least, since Lucien renamed them from "hot" to "p".

@yveskini
Copy link
Contributor Author

yveskini commented Dec 6, 2024

@drannawatts
For comment 1, how about giving a reason for wanting to change only the frequency? Something like this:

"One reason to adjust only the frequency in the model while keeping the data unchanged could arise from inadvertently using a frequency different from the one used to fold the data, only to realize this discrepancy after performing the run (with the wrong frequency)"

Comment 2: Fixed.

@drannawatts
Copy link
Member

I think that suggestion for Comment 1 is fine!

@thjsal
Copy link
Contributor

thjsal commented Dec 7, 2024

I made a few sentence changes (e.g. clarifying what importance sampling means ... hopefully it is correct?). Before moving this to the Documentation pages, I think it is still good to increase the font sizes in the Corner plots and clarify what is inside xpsi/utilities/ImportanceSample.py. I guess it is an alternative tool to use importance sampling developed by Serena. Maybe it is a bit weird that we have this alternative approach and we are not showing how to use that in the tutorial?

I can later try to fix these if you want.

@thjsal
Copy link
Contributor

thjsal commented Dec 13, 2024

I am working on this now.

Copy link
Member

@drannawatts drannawatts left a comment

Choose a reason for hiding this comment

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

I think this is looking good! Is it ready for approval or are people still making last changes?

@thjsal
Copy link
Contributor

thjsal commented Dec 13, 2024

Not ready! I just checked how it would look like in the docs, and it is a mess!

@thjsal
Copy link
Contributor

thjsal commented Dec 13, 2024

Need to make changes to the notebook titles, subtitles etc. so that they don't show up randomly in the docs and in the index.

Copy link
Contributor

@thjsal thjsal left a comment

Choose a reason for hiding this comment

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

I think this should be now more or less fine. The final change was to use * symbols instead of # to boldface normal text in the notebook. Otherwise, sphinx will treat those as titles and documentation becomes messy.

@thjsal thjsal merged commit 7b6df01 into main Dec 13, 2024
4 checks passed
@thjsal thjsal deleted the 87-importance-sampling-documentation branch January 10, 2025 07: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.

Importance sampling documentation
3 participants