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

Abins: Correct setting numerical zero for b_tensors. #18984

Merged
merged 9 commits into from
Feb 27, 2017

Conversation

dymkowsk
Copy link
Contributor

@dymkowsk dymkowsk commented Feb 23, 2017

  1. Fix bug with setting up b_tensors.

To test:

Code review

No issue number.


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?

  • How do the changes handle unexpected situations, e.g. bad input?

  • Has the relevant documentation been added/updated?

  • Is user-facing documentation written in a user-friendly manner?

  • Has developer documentation been updated if required?

  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

@samueljackson92
Copy link
Contributor

@martyngigg Might possibly want to take a look at this for the patch release.

@martyngigg martyngigg added the Indirect/Inelastic Issues and pull requests related to indirect or inelastic label Feb 24, 2017
@martyngigg martyngigg added this to the Release 3.10 milestone Feb 24, 2017
@martyngigg
Copy link
Member

@dymkowsk Thanks for the fixes.

There has been a system test failure for AbinsTest.AbinsCASTEPTestScale

Could you also include a note in the release notes for indirect?

@dymkowsk
Copy link
Contributor Author

@martyngigg, @samueljackson92 I just pushed changes you requested. Let me know if more modifications are needed.

@martyngigg
Copy link
Member

@dymkowsk Thanks. Can you remove the note in docs/source/release/v3.9.1.rst and just leave the one in 3.10.0. Sorry for not being clear.

If I pull this change into a patch then I will add that note myself. Removing that change will also remove the merge conflict.

@dymkowsk
Copy link
Contributor Author

@martyngigg I updated documentation as you wanted. I saw that one system test is still crushing. I changed the way reference data is compared (now .nxs vs workspace as for other tests). I can't reproduce this crushing on my Ubuntu.

@martyngigg martyngigg added the Patch Candidate Urgent issues that must be included in a patch following a release label Feb 27, 2017
@martyngigg martyngigg self-assigned this Feb 27, 2017
@martyngigg
Copy link
Member

@dymkowsk We have a test on Ubuntu that keeps sporadically failing even if you don't touch those files. We have not yet tracked these problems down however they are certainly not to do with these changes.

Code changes are acceptable and account for B-tensor-elements that are too small to be useful.

:shipit:

martyngigg pushed a commit that referenced this pull request Feb 27, 2017
Correct setting numerical zero for b_tensors.

(cherry picked from commit 6bd4aae)

Increase threshold.

(cherry picked from commit d3507cb)

Change criterion for validation: compare .nxs reference file with calculated workspace.

(cherry picked from commit e5c024a)

Add patch release note
@martyngigg
Copy link
Member

This has now been merged to master via merging the release branch so we may as well merge this now.

@martyngigg martyngigg merged commit 1f0a48c into mantidproject:master Feb 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Indirect/Inelastic Issues and pull requests related to indirect or inelastic Patch Candidate Urgent issues that must be included in a patch following a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants