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 WFM example in docs #49

Merged
merged 11 commits into from
Jul 4, 2024
Merged

Add WFM example in docs #49

merged 11 commits into from
Jul 4, 2024

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Jul 1, 2024

No description provided.

@nvaytet nvaytet requested a review from YooSunYoung July 1, 2024 13:26
Copy link
Member

@YooSunYoung YooSunYoung left a comment

Choose a reason for hiding this comment

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

I just have a couple of minor comments but it seems nice : D

And one more question regarding the target audience.
If the target audience already know about WFM and just wants to know more about the WFM stitching methodology, I think it is fine as it is,
but if it is about introducing WFM stitching methodology, I think it'll be nice to add more diagrams or pictures to explain the concept instead of just showing the code. But that can be done in another time.

"invalid_frames = True\n",
"fact = 0.01\n",
"while invalid_frames:\n",
" print(f\"Searching for bounds: threshold={fact}\")\n",
Copy link
Member

Choose a reason for hiding this comment

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

This part renders a bit funny.
Should it be rounded up?

image

"source": [
"## Find WFM frame edges\n",
"\n",
"To compute the frame edges, we use one of the openings in the WFM choppers are a time,\n",
Copy link
Member

Choose a reason for hiding this comment

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

I didn't get this sentence. Especially are a time part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it should be "we use one of the openings in the WFM choppers at a time" ;-)

@nvaytet
Copy link
Member Author

nvaytet commented Jul 4, 2024

I just have a couple of minor comments but it seems nice : D

And one more question regarding the target audience. If the target audience already know about WFM and just wants to know more about the WFM stitching methodology, I think it is fine as it is, but if it is about introducing WFM stitching methodology, I think it'll be nice to add more diagrams or pictures to explain the concept instead of just showing the code. But that can be done in another time.

For the target audience, I wanted to link to the old notebook we have that introduces the technique. However, there are 2 things to consider.

  1. I would like to move that notebook from that repo to a new one (probably scippneutron), so I felt like adding a link only to change it later was not great.
  2. That notebook uses the old ess code to perform the WFM correction to tof, and I didn't want to confuse users who see one way of doing it there, and another one here...

@YooSunYoung
Copy link
Member

Then we can add more link/general explanation later...!

@nvaytet nvaytet merged commit a8bff94 into main Jul 4, 2024
3 checks passed
@nvaytet nvaytet deleted the wfm-example branch July 4, 2024 14:34
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.

2 participants