-
Notifications
You must be signed in to change notification settings - Fork 2
Add inelastic sample component that changes neutron wavelengths #124
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
Merged
Merged
Changes from all commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
9f527aa
start modifying the way model runs
nvaytet 6f49224
update model to work with components
nvaytet b43a3e3
move more logic into components
nvaytet e23e753
fix imports
nvaytet 50da6f8
model runs and results are plotting
nvaytet 58f0b25
fix plotting of rays
nvaytet ddb0eb8
remove old reading file
nvaytet 7a8101d
start adding inelastic sample
nvaytet 1bf824c
fix deltae
nvaytet 0933abb
plot wavelength color for each section between components
nvaytet 4029e49
fix ray colors
nvaytet b2c6bed
add inelastic sample section to components notebooks
nvaytet 047d685
cleanup
nvaytet 87fefc4
fix plotting blocked rays
nvaytet a199784
start fixing tests
nvaytet acb156b
fix remaining tests
nvaytet d10610f
remove commented test
nvaytet 0e1e99d
fix dashboard
nvaytet 0d59007
add tests for inelastic sample
nvaytet cfde0c4
finish sample tests
nvaytet 13ab32c
static analysis
nvaytet 7201e0c
small style change
nvaytet c1aade2
use callable to compute deltaE for more flexibility, and prevent fina…
nvaytet 0c141a2
add test for final energy positive
nvaytet a5d209c
add rays in one go so that they share the same colorbar
nvaytet e1c4fc1
fix tests
nvaytet 504ad6e
Merge pull request #130 from scipp/fix-cbar-zoom
nvaytet 2435559
change function to return final energy
nvaytet 83e770b
update tests
nvaytet c585f81
format noteookb
nvaytet b54f955
update text in notebook
nvaytet cfb8093
add section on dealing with unphysical DeltaE
nvaytet 7011a02
prettier printing
nvaytet c85fcb3
NaN negative final energies instead of thresholding
nvaytet 00248de
delta_e -> func
nvaytet 3f17914
get the time_limit from neutron toas instead of passing it to all com…
nvaytet f6620c7
remove sample plot method
nvaytet da7261c
missing space
nvaytet 65425dd
remove tof property in component reading
nvaytet 29769fa
fix return type
nvaytet e2d2de4
replace _ with space
nvaytet 857c747
replace _ with space again
nvaytet 046f6c2
fix test name
nvaytet 542abe6
use assign instead of manually making shallow copies
nvaytet 6537cce
fix docstring for Model.add() method
nvaytet bca6503
fix Result.data to also add samples to the data group
nvaytet 54187fc
skip inelastic sample in json (de)serialization
nvaytet bb66427
component base class fixes
nvaytet 10af7f1
make kind an abstract property of components
nvaytet 3e49e2f
require exact match to extract components from model and result
nvaytet 7d460df
add sample __eq__
nvaytet 9f00518
add sample eq test
nvaytet 0d2290e
use assign methods instead of manually shallow-copying
nvaytet 11749a8
add edge cases tests with multiple samples and sample between choppers
nvaytet 84512e3
Apply automatic formatting
pre-commit-ci-lite[bot] ce486d0
remove unused imports
nvaytet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I"m wondering if it would make more sense to have a function take in the initial energy and return the final energy instead of the energy transfer?
It could look something like:
?
I initially went with returning
DeltaEso that the user writing the function would not have to worry about handling unit conversions and unphysical final energies. But the unit conversions are not too much code to add, and we can still fix final energies inside the package code after the final energy is returned by the function.This mechanism here would also allow the user to fix the negative energies in the way they want: either throw them away or give them a zero energy.
@bingli621 any opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, I went with the function that returns final energy, I think it makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the simulation's perspective, having Ef will make the calculation much easier. but the user should be able to change the energy transfer as well.
uniform_sampler = lambda size: rng.uniform(-0.2, 0.2, size=size)
def apply_energy_transfer(e_i, energy_transfer_sampler= uniform_sampler):
de = sc.array(dims=e_i.dims, values=sampler(size=e_i.shape), unit='meV')
return e_i.to(unit='meV') - de
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand.
The user supplied the whole function, so they can put in there what they want.
The question was: should the user supply a function that outputs energy transfer, or should it output final_energy?
In the first case, computing the final_energy from the initial_energy and the energy_transfer would be done internally in the package.
I think the second case make more sense; a function that would output the energy_transfer feels unusual/strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At T-REX and CSPEC, one sample can be measured with different Ei. In this sense, I would imagine user should supply a function describing the energy transfer, which is the property of the sample, regardless of what Ei is used to measure it. And the Sample component should stay the same when we change the condition how it is measured. I hope this what you are asking...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I am still applying a fix to the final energies here, but the function gives the opportunity to the user to fix it the way they want.
Is it safer to remove that check to avoid doing something silently that the user did not expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now added a section in the docs that shows how to manually deal with unphysical neutrons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation helps! I have two comments. The first is, tof.InelasticSample takes a keyword argument "delta_e", but this actually is a function that takes Ei as input and outputs Ef. This is what confused me. The logic is correct here, just the argument name is a bit misleading. The second comment is, when encountering the case with unphysical energy transfer, by replacing the negative Ef with a small ( 10^-30) positive value, we are basically disregarding them, because neutrons with such small Ef will pile up at a TOA value that is out of the time range of interests. In this sense, I suppose both ways to handle the unphysical Ef should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 1e-30 leads to an issue: the wavelengths are so large that it ends up being problematic to plot results (histograms and time-distance diagrams).
So I think by default I will NaN the numbers instead of thresholding at 1e-30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's find a better name for the argument.