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

Use dQ/|Q| for 2-D resolution #209

Merged
merged 1 commit into from
Mar 30, 2019
Merged

Conversation

pkienzle
Copy link
Contributor

@pkienzle pkienzle commented Mar 12, 2019

Custom pinhole smearing should use dQ/|Q| rather than dQ/Q [http://trac.sasview.org/ticket/1242](Ticket #1268).

@pkienzle pkienzle changed the base branch from master to release-4.2.2 March 13, 2019 19:26
@wpotrzebowski wpotrzebowski requested a review from butlerpd March 27, 2019 09:50
@butlerpd
Copy link
Member

ready for testing on Win64

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

Code looks right though it no longer will allow for dq_x and dq_y to be different (currently is not) but wonder if for some instruments (maybe TOF for example) that would not be a good assumption?

Otherwise functionality is tested and doe in fact fix the aberrant behavior. Suggests Andrew or Richard review regarding first point.

@butlerpd
Copy link
Member

Given it is only for pinhole presumably that may be OK .... unless you start worrying about gravity? So question remains if we want to allow asymmetry in dqx and dqy?

@prjemian
Copy link

prjemian commented Mar 29, 2019 via email

@prjemian
Copy link

prjemian commented Mar 29, 2019 via email

@pkienzle
Copy link
Contributor Author

pkienzle commented Mar 29, 2019

Any more complex resolution calculation is going to require more assumptions or more inputs. I created ticket 1271 (Issue #1290) which suggests defining resolution for the fit page from the resolution calculator panel.

Meanwhile, is the current fix sufficient to merge and close the ticket?

@ajj
Copy link
Member

ajj commented Mar 30, 2019

Looks fine to approve. Just make sure that #1290 has a reference to these commits to remind whoever works on it.

@pkienzle pkienzle merged commit d4cde37 into release-4.2.2 Mar 30, 2019
@pkienzle pkienzle deleted the ticket-1242-2d-resolution branch May 24, 2019 11:46
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.

4 participants