IDEMSInternational / R-Instat

A statistics software package powered by R
http://r-instat.org/
GNU General Public License v3.0
38 stars 102 forks source link

Completing the feature of having the multiple tables (html) in output window #8929

Closed N-thony closed 4 months ago

N-thony commented 5 months ago

Completes #8887 @rdstern this completes the work on having multiple tables in output window. This prevents repeating the same script (which produces multiple tables) multiple times in the output window.

rdstern commented 5 months ago

@Patowhiz this is needed by @Vitalis95 work on the 2/3 summarise dialog. It was done by @N-thony. Could you check and hopefuly merge this one. Thanks

Patowhiz commented 5 months ago

@rdstern @N-thony, could you provide an explanation of the original issue? Understanding the context and reasoning behind the change is crucial for me, especially since the code modifications here could impact other areas.

I recommend thorough explanations with clear steps for reproducibility related to such problems to always be standard practice. This will help in identifying other potential side effects.

Thanks.

N-thony commented 5 months ago

@rdstern @N-thony, could you provide an explanation of the original issue? Understanding the context and reasoning behind the change is crucial for me, especially since the code modifications here could impact other areas.

I recommend thorough explanations with clear steps for reproducibility related to such problems to always be standard practice. This will help in identifying other potential side effects.

Thanks.

@Patowhiz have a look here

Patowhiz commented 5 months ago

@N-thony, from the comment by @rdstern that you quoted, I'm still puzzled as to why a dialog would generate the exact same script multiple times. This seems indicative of an issue with the dialog itself, suggesting that improvements should be made there rather than in the R link. If you could provide more context on the code generated by the dialog, I would be able to offer guidance on where refactoring might be necessary.

N-thony commented 5 months ago

@N-thony, from the comment by @rdstern that you quoted, I'm still puzzled as to why a dialog would generate the exact same script multiple times. This seems indicative of an issue with the dialog itself, suggesting that improvements should be made there rather than in the R link. If you could provide more context on the code generated by the dialog, I would be able to offer guidance on where refactoring might be necessary.

@Patowhiz this is the context, you can see if you debug the code that since we have two paths leading to html output, it will keep adding the script too image

Patowhiz commented 5 months ago

@N-thony, the context is a Describe Two Variable dialog that generates two file outputs after executing its R command. While the R link has been refactored to handle a collection of file paths, the output logger hasn’t been updated accordingly. This discrepancy led to the addition of a loop in the R link to add the outputs with the same R command.

To resolve this, the output logger and its dependencies need to be refactored to manage a collection of outputs post-R command execution.

I recommend starting by assessing how lines 1015 to 1030 could be integrated into the output logger. Specifically, the AddOutput() subroutine of the output logger should be modified to expect a collection of outputs. This change will likely require refactoring the current parameters to support the new structure.

I hope this guidance is helpful.

Thanks.

N-thony commented 5 months ago

@N-thony, the context is a Describe Two Variable dialog that generates two file outputs after executing its R command. While the R link has been refactored to handle a collection of file paths, the output logger hasn’t been updated accordingly. This discrepancy led to the addition of a loop in the R link to add the outputs with the same R command.

To resolve this, the output logger and its dependencies need to be refactored to manage a collection of outputs post-R command execution.

I recommend starting by assessing how lines 1015 to 1030 could be integrated into the output logger. Specifically, the AddOutput() subroutine of the output logger should be modified to expect a collection of outputs. This change will likely require refactoring the current parameters to support the new structure.

I hope this guidance is helpful.

Thanks.

@Patowhiz it makes sense, have a look. Thanks

N-thony commented 5 months ago

@N-thony, this is a good start.

However, in line with the original concept, each output element should be associated with an R script and its corresponding output(s). Given that an R command can produce multiple outputs, it's necessary to refactor the output element to handle a collection of outputs instead of setting empty R command strings.

This adjustment also implies that you will need to refactor the output window to accommodate this change. Essentially, output window and it's components should be capable of linking to a single output element that contains a collection of outputs.

Before you begin the implementation, I recommend reviewing all the data structures that comprise the output system at the VB.Net level. Keep in mind that these output elements are intended to be utilised by the script window in the long run.

I hope this information is helpful.

Thanks.

@rdstern @Patowhiz while waiting for what we will decide in #8948 I suggest that we get this PR and #8932 merged, what do you think?

N-thony commented 4 months ago

@N-thony, this is a good start. However, in line with the original concept, each output element should be associated with an R script and its corresponding output(s). Given that an R command can produce multiple outputs, it's necessary to refactor the output element to handle a collection of outputs instead of setting empty R command strings. This adjustment also implies that you will need to refactor the output window to accommodate this change. Essentially, output window and it's components should be capable of linking to a single output element that contains a collection of outputs. Before you begin the implementation, I recommend reviewing all the data structures that comprise the output system at the VB.Net level. Keep in mind that these output elements are intended to be utilised by the script window in the long run. I hope this information is helpful. Thanks.

@rdstern @Patowhiz while waiting for what we will decide in #8948 I suggest that we get this PR and #8932 merged, what do you think?

Any updates here?

Patowhiz commented 4 months ago

@N-thony, I'm still waiting for responses on issue #8948. Depending on the feedback from the broader team, we can decide whether to evaluate the approach taken here or redo it based on the agreed approach.

I'll reach out to @ChrisMarsh82 and @rdstern to expedite discussions and decisions on the issue. @rdstern, if you believe that resolving this issue is more urgent than the discussions on issue #8948, feel free to overrule me on this PR, then I can merge.

Vitalis95 commented 4 months ago

@Patowhiz ,this is needed in 2/3 var summaries, Could you merge ?

Patowhiz commented 4 months ago

@Vitalis95 thanks for the reminder.

@rdstern I was hoping I would get a quick feedback on issue #8948 but there has been no response on the team tagged in the issue. Considering the necessary decision is beyond us, should we go ahead and merge? We can later look into how this can be amended once we get a decision in that issue.

@N-thony in the meantime, you could resolve the conflicts then once @rdstern tests and decides that we should merge, I will go ahead and merge. Thanks.

rdstern commented 4 months ago

@Patowhiz I agree to go ahead, get @N-thony to resolve conflicts and merge for now. I think I approved before and will be happy to check again. I like your question in #8948 but it is beyond my "pay grade". This does seem to be a topic for you and Chris to advise on, so I suggest it could be good to add to the Thursday discussion? I wonder if it is even directly relevant, as gt has many formats for the results?

N-thony commented 4 months ago

@Patowhiz I agree to go ahead, get @N-thony to resolve conflicts and merge for now. I think I approved before and will be happy to check again. I like your question in #8948 but it is beyond my "pay grade". This does seem to be a topic for you and Chris to advise on, so I suggest it could be good to add to the Thursday discussion? I wonder if it is even directly relevant, as gt has many formats for the results?

@rdstern I resolved the conflicts.

N-thony commented 4 months ago

@rdstern I realized one of the compilecheck failed which leads to a build error if you did run before the fix now. Have a look.