-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add data parameters #28
Conversation
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.
Great job! I added some points that can be improved. Also you might need to update the unit tests since there are new parameters now
I have corrected the issues and added the last data parameters. Moreover, for the visual parameters for RDAPlot/RowTreePlot (tip_size_by/edge_size_by/node_size_by). It is needed to perform some modification to the data pefore plotting so I don't know if this is something to be added (also how). |
Nice progress! For the size parameters of RDAPlot and RowTreePlot, we can just provide the parameter without adding code. Note that a simple example like this also works:
If you think that some more advanced usage should be explained (like the code you wrote above), the right place is in |
I have added example for Row data so I'll explain what's the issue: |
Now all the visual parameters should have been added in both RDAPlot & RowTreePlot (maybe there can be a better way to sort these parameters in RowTreePlot, I can rework this). |
Fixed. |
…nto dataparameters
Ideally, we would like to adjust, say, line colour, while keeping line size unchanged. But in the current configuration one cannot activate "colour by" without also activating "shape by", or "size by". Probably the checkbox solution (colour, shape, size) as in the RDAPlot panel could fix this. |
I have made changes for checkboxes but there are still issues :
|
At least the first issue is likely due to this: say you turn on colour, set it to blue, and turn it off. Once you turn it off, the conditional in the .generateOutput method will return FALSE so colour will not be changed (will stay blue). Thus, we might need to look how it is done originally in the source code of iSEE visual box, or come up with something smart ourselves. |
In the reducedDimPlot there is quite the same issue (except that sometimes there is a conditional radio button after turning on the checkbox so it can be disabled), but app shouldn't crash at least |
Right! Two things:
|
Everything should be working I believe now, let me know if you find anything wrong still |
Thank you! We are getting closer. We would like also colour, size and shape of, say, node to be independent from each other, so that we can colour nodes without turning on their size. How about the following configuration? (sorry for switching again) [ ] Color [ ] Shape [ ] Size Color by: Shape by: Size by: By default, nothing is active. When you turn on for example colour, you will see options line, tip and node. You can choose to colour only one of them, or more, but once you turn off the corresponding checkbox, say line, the colour of line should go back to default (turn off line colour). It's a good idea to build it up gradually and make sure it works for a smaller subset of params. If it does, then scale it up to all remaining options. Do you think this can be a good solution? Let me know if you need any help. Btw, currently colour is working only for edge and not for line and tip. |
Also if you want maybe you could add yourself to the contributors in DESCRIPTION? |
The rework has been made, there shouldn't be any crash and should be operational. |
Like it does not refresh the plot when desactivating a checkbox which is probably the reason behind |
Ok, I think it's good to go. If we find a solution to turn off aesthetics we can add it in the future. Let's fix the checks and then we can merge. |
Tests somehow fail in the github workflow but all tests were successful locally, probably takes the previous tests version |
Tests are probably failing because we are using the GitHub version of miaViz. @TuomasBorman, when will it be pushed to bioconductor devel? |
There are big changes both in mia and miaViz. I pushed now, but there might occur some issues since miaViz is not yet synced with mia |
This PR aims to follow the process of solving issue #25.
Data parameters added:
Current issues :
confidence.level (RDAPlot) is a numeric between 0 and 1, so I'm using a sliderInput but even after adding step parameter there is only 0 and 1 that can be selected, moreover it doesn't seem to change anything on the plot either.
shape_by (AbundanceDensityPlot) should be hidden when layout="density", the only method for this looks to be .conditionOnRadio but it should only work on radiobuttons & checkboxes (not selectInput). I have two solutions : creating a method for selectinputs or change the selectinput in radiobuttons?
Still to implement :
Note : Have to be using the GitHub version of miaViz