Replies: 21 comments 52 replies
-
|
I actually like the |
Beta Was this translation helpful? Give feedback.
-
|
I prefer |
Beta Was this translation helpful? Give feedback.
-
|
The I understand the utility for the |
Beta Was this translation helpful? Give feedback.
-
Both options will have to be supported in a transition-deprecation period but in a long term we should decide on a single way. Maintainint both require extra logic to maintain. |
Beta Was this translation helpful? Give feedback.
-
|
I was thinking about that are we going to achieve and what are the options. I will think more about above decision tree. If we don't touch sense of tm_xy deeper then the three could be like this: 
Or more complex 
|
Beta Was this translation helpful? Give feedback.
-
|
I have a question about potential teal::init(
modules = modules(
module(label = "m1", ...),
module(label = "m2", ...)
),
transform = teal_transforms(
teal_transform(id = "t1", ...),
mapping = list(
m1 = "t1",
m2 = "t1"
)
)
)Do we want them to be shared or just copied. I mean this:
|
Beta Was this translation helpful? Give feedback.
-
|
Considering that in |
Beta Was this translation helpful? Give feedback.
-
|
Just to clarify, is |
Beta Was this translation helpful? Give feedback.
-
|
Yup, sorry my bad - I've fixed the code snippet |
Beta Was this translation helpful? Give feedback.
-
|
Small note that mapping by label is very confusing. This should have been done by ID. There is a dedicated issue for this: #969. If done, there will be no problems with duplicated labels (which should be perfectly fine btw). |
Beta Was this translation helpful? Give feedback.
-
|
I might agree to "mapping by label is very confusing" but not to "this should have been done by ID". |
Beta Was this translation helpful? Give feedback.
-
|
My preference is towards by argument. I think it's simpler. I would like to have just one collection to manage (that is what is passed to the This is what I am talking about: There are few bad consequences of this. I think that the most important is that it would make incremental app building very difficult. Let's suppose that the app throws an error. A typical debugging includes commenting-out modules and add them one by one to identify which module throws. You cannot do this easily as you need to comment-out / uncomment two collections. That's really unnecessary I believe. |
Beta Was this translation helpful? Give feedback.
-
|
This is why I prefer mapping by app (global) and module (local) arguments and allow inclusion of |
Beta Was this translation helpful? Give feedback.
-
|
In this comment I use a certain shortcuts to describe two type of handling transform modules:
This comment is written in a "iterative" way, so that I start from the simple proposition and trying to improve step by step. 1. Transform only in the modulepseudo apiinit(
filter = teal_slices(
mapping
),
modules = modules(
module(transforms)
)
)In the above a module encoding can grow significantly. Module developer will have extra responsibility to handle data transform. 2. Transform handled by tealinit(
filter = teal_slices(module_specific),
modules = modules(
module() |>
with_transforms()
)
)Instead of handling in a module we can handle this in teal. This will be done by adding 3. No
|
Beta Was this translation helpful? Give feedback.
-
I mean the transform panel would be like the filter panel, it would act before data (
There is no such thing as a global condition, though. There are only filtering conditions, some of which happen to be shared by all modules. But essentially yes. Have an intermediary agent that will collect user-defined transform instructions and evaluate them before reconstructing |
Beta Was this translation helpful? Give feedback.
-
|
We may be repeating ourselves because we are posting at the same time.
Is not the same if |
Beta Was this translation helpful? Give feedback.
-
|
The part where we specify init(
filter = teal_slices(...),
transform = teal_transforms(...)
)and we have a unified API for filters and transformers for me is the most intuitive. In both cases we do not mess up with modules code and modules parameters (which both are already long and complicated). I don't yet see the complication of filter being global (module_specific = FALSE) and transform local (module_specific = TRUE) but if this is the problem, maybe we can move |
Beta Was this translation helpful? Give feedback.
-
It is different, when it is module_specific then filter panel is created once outside of the modules and it is truly single filter-panel
@m7pr I agree that api is intuitive (I ignore mapping limitation for duplicated module labels). Also, as an app developer might be hard for you to understand why in the previous (module_specific) app filters are added (and removed) only to the single module and in the other (!module_specific) filters are added to every module. |
Beta Was this translation helpful? Give feedback.
-
Yeah, I think it works the same right now. But I imagine this might be more difficult with another dimension (for transformations). So maybe it's not that ideal |
Beta Was this translation helpful? Give feedback.
-
This looks like an extreme case to me. Would it not mean that one module operates on essentially different data? Perhaps the app dev should create |
Beta Was this translation helpful? Give feedback.
-
Yes, it is an extreme case. There is even more extreme.
What do you mean by this? In all cases which include module-specificity modules operate on a different data.
Should or could? App developer can do anything with the data up to the moment when teal_module is called |
Beta Was this translation helpful? Give feedback.
-
Yes, but it is different only after exiting the filter panel. What I mean is to create a second
Excellent question. Maybe part of the functionality is already available but requires creative usage of |
Beta Was this translation helpful? Give feedback.
-
Right, I remember now.
This is true before the introduction of transforms. I respectfully suggest not combining it with the problem at hand unless serious changes in |
Beta Was this translation helpful? Give feedback.
-
@chlebowa could you elaborate more? |
Beta Was this translation helpful? Give feedback.
-
|
I believe you are referring to the fact that the mapping matrix and the available filters dialog distinguish filter states because they print their id, whereas the filter cards do not because they only display variable name and variable label. This is a problem already, even before without transforms. It is a separate issue IMHO. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, but it is important to include this in this discussion also. This is double sided sword. On one side
|
Beta Was this translation helpful? Give feedback.
-
|
Question from the meeting:
|
Beta Was this translation helpful? Give feedback.
-
|
If one adds a filter that is attached to all modules, can it be later detached from only one module? Can a local filter exist for the same variable? |
Beta Was this translation helpful? Give feedback.
-
|
My first thoughts
When clicking remove there will be "remove here", "remove everywhere"
Still no |
Beta Was this translation helpful? Give feedback.
-
|
Hey guys, with a great power of Power Point I created a small proposition of the UI and API in here #1223 |
Beta Was this translation helpful? Give feedback.
-
What's the advantage of having Let's say I think we could have sidebars defined outside (maybe cache reactive result to avoid duplication) The approach on My general concern is about the complexity. I share @gogonzo thoughts on the reactives (which might not be restricted to The idea of @m7pr on making this drag-and-dropable is enticing as there will probably be no technical limitations to such a feature if we set up the structure correctly (there will be edge cases such as where moving operation above the creation on new variable, but we could deal with them). Some questions: 🯄: Could we make use of 🯄: Should transform operations be allowed on existing columns? let's say |
Beta Was this translation helpful? Give feedback.
-
|
Agree that 6 gives the most flexibility and customization! |
Beta Was this translation helpful? Give feedback.
-
I have one biggest question about a solution. If input (reactive x) of item2 depends on the output of the item1 what happens if the item1 is removed?
I don't think so as output of itemN will depend on independent serie from item1 to itemN-1.
It will be impossible to find robust solution to forbid this. Transform could be custom shiny module and parsing/detecting calls or monitoring environment would be hard.
Changing an order in the interactive session is similar problem to the remove and add. We need to first figure out if we can recompute reactive structure on the fly. I can imagine a solution in which input of itemN is something like this # pseudocode
reactive_x <- reactive({
idx_this <- <reactive value>
idx_previous <- idx_this - 1
previous_item <- reactive_queue[[idx_previous]]
previous_item()
})
transfrom_item_srv(reactive_x, ....)Above is mind-boggling as item's module needs to be called once with some reactive. This means that reactive can't be simply output of previously called module, but an output of a module located as previous in the queue. So any change of the order or previous items itself should retrigger reactivity. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, changing the order with drag-and-drop or adding a new one / removal of the old one, should just re-trigger the reactivity chain, which might not be ideal when: you create a new transformer and then you drag-and-drop it to top, which triggers the interativitiy twice. |
Beta Was this translation helpful? Give feedback.
-
|
Basically, we have few proposed ways of how to map transformation items with modules.
My conclusion: |
Beta Was this translation helpful? Give feedback.
-
I think this is acceptable. Most of the time,
So, the issue is during the global mapping. This means that the transform module can be applied globally, but it remains independent of each module, unlike teal_slices. If we're adopting the same mapping style with |
Beta Was this translation helpful? Give feedback.
-
|
The majority of the vote prefers the What about the design like this? add_select <- teal_transform_module()
add_groupby <- teal_transform_module()
add_distribution <- teal_transform_module()
add_summarize <- teal_transform_module()
app <- init(
data = teal_data(
iris = iris
),
modules = modules(
module(label = "my module 1") |>
add_select(default = c("armcd")) |> # select default input
add_summarize(default = c("age", "sex", "race")),
module(label = "my module 1") |>
add_select(default = c("arm")) |> # select different default input for this module
add_groupby() |>
add_distribution()
),
title = "my teal app",
filter = teal_slices(
teal_slice()
)
)
shinyApp(app$ui, app$server)Note that I'm not including any global transformations. I'm still not 100% sure if global transformations are a use case we want to fulfill. I think the transform module should be limited to specific modules. As long as we can reuse the module, I think that's good enough. |
Beta Was this translation helpful? Give feedback.
-
This is why I feel we should place only one "global" filter/transform pannel while only module-specific filters/transforms are present inside the module tab. Only the active filters are important to display inside the module even if it is a global filter.
I think it should. I think that we can create custom transform widgets in |
Beta Was this translation helpful? Give feedback.
-
Isn't |
Beta Was this translation helpful? Give feedback.
-
True, perhaps it can live under a different roof. But, I see a benefit of providing these transform modules. |
Beta Was this translation helpful? Give feedback.
-
There will be a lot to educate.
Yup, this is also something to consider. @pawelru for example don't like decorating To all, don't get me wrong, I love shared teal_slice, but when custom
|
Beta Was this translation helpful? Give feedback.
-
|
I think that's okay, given that it would be even more confusing to re-educate the users as your pointed out before. |
Beta Was this translation helpful? Give feedback.
-
|
I also think having
Above will make filter panel more clear |
Beta Was this translation helpful? Give feedback.
-
|
I'm fine if this is always module_specific - at least it's consistent, and then we have transformations that are local (only applied to the specific module). |
Beta Was this translation helpful? Give feedback.
-
|
I am leaning toward supporting this too. If we're making filter panel to always be module specific, would it make sense if filter panel comes after the transform module fo r each module? flowchart LR
A[transform module] -->|teal_data| B[Filter Panel]
B -->|teal_data| C[teal module]
This way the filter panel will always show the information after data is transformed. |
Beta Was this translation helpful? Give feedback.
-
How about we don't combine them? If we don't combine them, the worst scenario I can imagine is app developers writing a teal_transform_module for filtering. However, we should discourage this, as filtering should be done with the filter panel. In fact, if we don't combine them, there will be a clear distinction between the two: users will go to teal_slice for filtering and to the transform module for transformation. |
Beta Was this translation helpful? Give feedback.
-
|
Possible ways without going into details:
|
Beta Was this translation helpful? Give feedback.
-
I think this is OK for the first approach. Maybe if we get any user feedback in the future, we can iterate and add a couple more modifications in the future. |
Beta Was this translation helpful? Give feedback.
-
Sounds a bit complicated even for core-dev members. So yeah, imagine module or app developer utilizing this :) |
Beta Was this translation helpful? Give feedback.
-
I think exactly the opposite, this is not ok for the first attempt. If we decide to make (2), then to allow custom transformation in the future we would need to add (4). |
Beta Was this translation helpful? Give feedback.
-
|
And what's wrong about it? Going from 2) -> 4) but still do not allow transformations to be shared |
Beta Was this translation helpful? Give feedback.
-
I think this will make us come back to the quest to find a more robust solution again. At first I was fond of this idea, but after having conversations around a general transform API I do not like it that much now.
From my perspective this is a better approach. Provides the strongest distinction between global and local filters. Although after comments from @donyunardi (quote from Dony 👇🏽) I'm not sure if we need a global transform. Technically, such a global transform can be performed by
|
Beta Was this translation helpful? Give feedback.
-
|
As a framework, providing more flexibility is better, and Here we can deprecate mapping for slices/tranform defined in the global scope by combining approach 3.
|
Beta Was this translation helpful? Give feedback.
-
|
The user can define the order and lock it and it will remain for the entire session. developer can always define the order for both local and global. |
Beta Was this translation helpful? Give feedback.
-
|
Pseudo API: API has an
Local Filters/Transformations: |
Beta Was this translation helpful? Give feedback.
-
|
transforms and slices items can be wrapped by the same function, doesn't have to be teal_transforms and teal_slices. We can have init(
transforms = foo(
teal_slice(),
teal_transform(),
...
),
modules = module() |> with_foo(
teal_transform(),
teal_slice(),
...
)
) |
Beta Was this translation helpful? Give feedback.
-
|
I also want to think about a potential problem that will arise due to the increase in widgets that we are talking about. For example, right now the module encoding validation can provide some shiny validations else we end up with generic qenv errors. I think we should have a space for such validations for the transformations too. The disconnect I want to point out is that module developer has the information about this, but app developer is potentially the one who can add this in the transform server. |
Beta Was this translation helpful? Give feedback.
-
I consider above valid as it was agreed on multiple meetings with everyone involved.
teal::init(
data = ...,
modules = modules(
# by arg
tm_xy(..., transforms = <list of teal_transform_module>),
# by decorator
tm_xy(...) |> with_transforms(<list of teal_transform_module>)
)
)
Because we can't mix |
Beta Was this translation helpful? Give feedback.
-
What decision needed here? I'm under the impression that we're moving away from
|
Beta Was this translation helpful? Give feedback.
-
|
I'm reading through the comments again and I think I understand point 2 a bit more. I believe we're trying to reuse the concept of example_module(
label = "my module",
transform = list(
teal_slice(),
teal_transform_module()
)
)The questions is, do we have to? I think we should keep the filtering mechanism separate with transformation mechanism. |
Beta Was this translation helpful? Give feedback.
-
This is separate discussion if we want to change |
Beta Was this translation helpful? Give feedback.
-
|
Closing discussion. We start with simple |
Beta Was this translation helpful? Give feedback.
-
What API you prefer for data transformation in future
teallet's vote, discuss and revote.teal::init( modules = modules( tm_xy(...) # no transform here ), transform = teal_transform( ..., mapping = [list named after module labels] ) )Pros:
Cons:
tea::init( modules = modules( tm_xy( ..., transform = teal_transform(...) ) ), transform = teal_transform(...) # no mapping anymore )Pros:
Cons:
Key takeaways from the discussions:
reactive(teal_data)as input and output of the transform module is a good solution.teal_transform_modulecan't be shared across the modules as same shiny module is called independently.7 votes ·
Beta Was this translation helpful? Give feedback.
All reactions