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

Amor reflectometry #32

Merged
merged 12 commits into from
Apr 23, 2021
Merged

Amor reflectometry #32

merged 12 commits into from
Apr 23, 2021

Conversation

arm61
Copy link
Contributor

@arm61 arm61 commented Apr 21, 2021

Companion to scipp/ess#15, showing the reduction process for the Amor instrument in action

@arm61 arm61 closed this Apr 22, 2021
@arm61 arm61 reopened this Apr 22, 2021
@arm61
Copy link
Contributor Author

arm61 commented Apr 22, 2021

Closed and reopened to trigger CI.

@arm61 arm61 closed this Apr 22, 2021
@arm61 arm61 reopened this Apr 22, 2021
@arm61
Copy link
Contributor Author

arm61 commented Apr 22, 2021

🎉

@SimonHeybrock
Copy link
Member

Unless there is a very specific reason, we let Sphinx auto-generate the notebook outputs (using nbspinhx). Before commiting a notebook, always run "Restart kernel and clear all outputs" in the menu, then save, and then commit.

@arm61
Copy link
Contributor Author

arm61 commented Apr 23, 2021

Good point. I am used to putting notebooks that run for > 5 minutes into docs so time out nbsphinx!

reflectometry/amor_reduction.ipynb Outdated Show resolved Hide resolved
Comment on lines +191 to +192
"bins = (np.linspace(2.5, 15, 50), np.linspace(0.6, 1.25, 50))\n",
"lambda_theta = norm.wavelength_theta_bin(bins)"
Copy link
Member

@SimonHeybrock SimonHeybrock Apr 23, 2021

Choose a reason for hiding this comment

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

This interface should be changed (in the future). As it is it negates all the benefits we have from scipp, making things obvious and safeguarded from errors:

  • Which of the two refers to what?
  • What are the units?

Similar for a number of other functions I think, but we can do that as a next step (but we actually have to do it, and avoid getting carried away by a flood of feature requests and pressure to continue implementing new things before consolidating what we have).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, ideally this would be replaced with two sc.linspace calls 😉 (scipp/ess#15 (comment))

reflectometry/amor_reduction.ipynb Outdated Show resolved Hide resolved
@SimonHeybrock SimonHeybrock merged commit 86f7c19 into scipp:master Apr 23, 2021
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