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

fixing rounding issue in min-max slider #1116

Merged
merged 9 commits into from
Jul 10, 2024
Merged

Conversation

evelyn-schmidt
Copy link
Contributor

@mhoang22 encountered a problem where the dynamic scatter doesn't show all candidates for the custom module. In her pvacsplice output, there were 5 candidate groups (corresponding to 5 genes associated with the neoantigens). She expected to see 5 dots corresponding to the 5 candidate groups. However, only saw 3 candidates. When examining the ‘Overview of Neoantigen Features’ board again, it looks like the 2 candidate groups with extreme VAF values (1st candidate: gene ATG12: tumor DNA VAF 0.432 , tumor RNA VAF 0.387; 5th candidate: gene TP53: tumor DNA VAF 0.852, tumor RNA VAF 0.998) are the missing candidates. If she natural-logged transformed both x and y axis, then she could see 5 candidates. She concluded that there was a reason that candidates with edge values were being excluded.

I found that these edge cases were being missed because of the rounding that happens to create the min-max slider. The range used for the min-max slider was originally rounded and now is just set as the dataset min/max.

min_value <- as.numeric(format(round(range_values[1], 2), nsmall = 2))
max_value <- as.numeric(format(round(range_values[2], 2), nsmall = 2))

So the change looks like this:

min_value <- round(range_values[1], 10)
max_value <- round(range_values[2], 10)

Evelyn Schmidt added 3 commits June 20, 2024 15:45
Copy link
Contributor

@susannasiebert susannasiebert left a comment

Choose a reason for hiding this comment

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

We could also consider using floor and ceiling in order to round down (for the min_value) and up (for the max_value), respectively. Not sure if, for display purposes, an int number is preferred, e.g. if the min_value and max_value are displayed on the chart axes and sliders.

@evelyn-schmidt
Copy link
Contributor Author

Yeah, @susannasiebert , I like the floor/ceiling suggestion because it looks a little cleaner on the slider.

However, is the most clear option for users to set the max/min value to the actual min/max value in the dataset? So you can see the maximum and minimum values on the slider?

@susannasiebert
Copy link
Contributor

I personally don't think they need to match. They can always retrieve the actual min max value by hovering over the appropriate datapoint, right?

@susannasiebert susannasiebert changed the base branch from master to hotfix June 24, 2024 13:44
Copy link
Contributor

@ldhtnp ldhtnp left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. I agree with using floor and ceiling to have cleaner values displayed on the slider. I provided suggestions to add this

pvactools/tools/pvacview/server.R Outdated Show resolved Hide resolved
pvactools/tools/pvacview/server.R Outdated Show resolved Hide resolved
pvactools/tools/pvacview/server.R Outdated Show resolved Hide resolved
pvactools/tools/pvacview/server.R Outdated Show resolved Hide resolved
pvactools/tools/pvacview/server.R Outdated Show resolved Hide resolved
pvactools/tools/pvacview/server.R Outdated Show resolved Hide resolved
evelyn-schmidt and others added 6 commits June 24, 2024 14:02
Co-authored-by: Luke Hendrickson <64616402+ldhtnp@users.noreply.github.com>
Co-authored-by: Luke Hendrickson <64616402+ldhtnp@users.noreply.github.com>
Co-authored-by: Luke Hendrickson <64616402+ldhtnp@users.noreply.github.com>
Co-authored-by: Luke Hendrickson <64616402+ldhtnp@users.noreply.github.com>
@susannasiebert susannasiebert merged commit c146174 into hotfix Jul 10, 2024
6 checks passed
@susannasiebert susannasiebert deleted the pvacview_fixes branch September 12, 2024 13:48
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.

3 participants