-
Notifications
You must be signed in to change notification settings - Fork 50
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
2039 red blood cells #2058
2039 red blood cells #2058
Conversation
|
||
chart.SeriesTemplate.NumericSummaryOptions.SummaryFunction = | ||
$"{STR_CUSTOM_SUMMARY_FUNCTION_NAME}([{_fieldNormValue.FieldName + "_" + _fieldNormValue.SummaryType}],[{_fieldNormValue2.FieldName + "_" + _fieldNormValue2.SummaryType}])"; | ||
// // Create argument descriptions for the summary function. |
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 is just required for now so that I can get some values. We can uncomment when the problem is solved with query
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.
@Yuri05 I think this fixes the issue. No changes of default value. will do it next
{ | ||
//special case for blood cells and vascular endothelium. if the value is set > 0. we need to turn on some locations programatically | ||
var bloodCells = _queryExpressionResults.ExpressionResultFor(CoreConstants.Compartment.BLOOD_CELLS); | ||
if (bloodCells?.RelativeExpression > 0 && !protein.IsBloodCell()) |
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.
why do we need the part && !protein.IsBloodCell()
?
(same question for VascEndo below)
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 logic that I have in mind is the following: If Bloodcells was already activated (Membrane OR Intracellular is on), then the field was set by the user and the rel exp is visible. Therefore, I do not want to trigger a command that willl reset potentially some fractions etc...
In short, I just want to ensure that the rel exp field is made visible only if it was not already visible.
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 logic seems ok :)
But I still don'T understand the highlighted part. It checks if the protein is named "Blood Cells". Why?
I guess you mean !protein.InBloodCell()
??
And also for VascEndo !proteinInVascularEndothelium
and not !proteinIsVascularEndothelium
?
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.
you are very much correct. I changed the code locally without saving and it did not commit those changes. I am going to add test for this use case where it's already set :)
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.
Very good eye @Yuri05 Thanks for catching this.
I added a new test testing the second part of the if for VascularEndo and RBC . We are good
50bc173
to
6df2d6b
Compare
} | ||
|
||
[Observation] | ||
public void should_not_turn_on_vascular_endosome() |
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.
new test which was failing with the bug you found in the implementation . Kudos
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.
ready to merge I think
looks good :) |
No description provided.