-
Notifications
You must be signed in to change notification settings - Fork 9
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
Bugfix/145 bug found in dmcdrofrhodmusdetector #146
Bugfix/145 bug found in dmcdrofrhodmusdetector #146
Conversation
…n dMuaDetector with comments. Renamed unit tests for DAW and CAW OneLayerDetectors to include "dMC" in name because added derivative detector tests.
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.
Thanks for fixing this @hayakawa16
…dentical code. That is because derivative Radon-Nikodym factor is same for DAW and CAW.
While fixing prior sonarcloud new issues, I've introduced 2 new issues. I think I need @lmalenfant advice on how to fix 2 new issues. |
I think you need to add "break;" after each case |
…n weighting type: 1) added Analog option so switch encompasses all enum types, and 2) removal of the empty case clause.
…log and the resulting exception.
Quality Gate passedIssues Measures |
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.
Looks good, I can't see the granular changes in the test files but I think that's ok because they are tests.
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.
@hayakawa16 Do you know why this file is not showing individual changes? Do you think it might be a line ending issue?
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 renamed the file from pMCCAWOneLayerDetectorsTests.cs to pMCdMCCAWOneLayerDetectorsTests.cs to indicate that it includes unit test for the dMC derivative detectors using CAW as well as the original pMC perturbation detectors. Possibly the reason?
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.
Yes that could be it. I approved so you can merge when you're ready.
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.
Thanks @lmalenfant! I will do so. I will also put some documentation about the derivative equations up tomorrow hopefully.
public void Test_Analog_absorption_weighting_type_throws_argument_exception() | ||
{ | ||
var test1 = new DMuaDetectorTest(); | ||
Assert.Throws<ArgumentException>(() => |
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.
Nice!
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.
This file is also not showing individual changes.
I think this is ready for review. @janakarana made a lot of simplifications to the code equation-wise. I simplified the code logic.