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

Model > One Variable > Fit Model dialog produced script #5995

Closed
Patowhiz opened this issue Sep 23, 2020 · 11 comments
Closed

Model > One Variable > Fit Model dialog produced script #5995

Patowhiz opened this issue Sep 23, 2020 · 11 comments
Assignees
Milestone

Comments

@Patowhiz
Copy link
Contributor

The script produced by the dialog has redundant R code. think some of the lines of code can be omitted.

# Code generated by the dialog, One Variable Fit Model

tmax <- data_book$get_columns_from_data(data_name="dodoma", col_names="tmax")
last_model <- fitdistrplus::fitdist(data=as.vector(x=stats::na.exclude(object=tmax)), method="mle", distr="norm")
data_book$add_model(model_name="last_model", model=last_model, data_name="dodoma")
data_book$get_models(data_name="dodoma", model_name="last_model")
last_graph <- graphics::plot(x=data_book$get_models(data_name="dodoma", model_name="last_model"))
data_book$add_graph(graph_name="last_graph", graph=last_graph, data_name="dodoma")
data_book$get_graphs(data_name="dodoma", graph_name="last_graph")
rm(list=c("last_model", "tmax", "last_graph"))

I think line

 data_book$get_models(data_name="dodoma", model_name="last_model")
data_book$get_graphs(data_name="dodoma", graph_name="last_graph")

could be unnecessary.

@Patowhiz
Copy link
Contributor Author

@Ivanluv I've been told by @rdstern you are looking into enhancing the dialog, probably you can have a look at what's causing this redundant scripts to be produced.

@Ivanluv
Copy link
Contributor

Ivanluv commented Sep 23, 2020

Yeah, I am to make improvements to the dialogue .I will fix this when making the improvements.However, I cant work on this now since PR#5861 has changes on the same dialogue

@rdstern
Copy link
Collaborator

rdstern commented Sep 23, 2020

That's right. We need to wait for #5861 to be merged first.

@Ivanluv
Copy link
Contributor

Ivanluv commented Oct 5, 2020

@Patowhiz I have just tried looking at this and the last line, data_book$get_graphs(data_name="dodoma", graph_name="last_graph") seems to be needed for the graph to be displayed

@Patowhiz
Copy link
Contributor Author

@Ivanluv okay, what about line data_book$get_models(data_name="dodoma", model_name="last_model") ?

@maxwellfundi maxwellfundi added this to the 0.7.0 milestone Mar 15, 2021
@Ivanluv Ivanluv moved this from To do to In progress in R-Instat Sprint March 2021 Mar 15, 2021
@Ivanluv Ivanluv moved this from In progress to To do in R-Instat Sprint March 2021 Mar 15, 2021
@Ivanluv
Copy link
Contributor

Ivanluv commented Mar 17, 2021

@Patowhiz data_book$get_models(data_name="dodoma", model_name="last_model") is used to print the results in the output ie

Fitting of the distribution ' norm ' by maximum likelihood 
Parameters:
     estimate Std. Error
mean    4.486     0.5184
sd      3.110     0.3665

Also data_book$get_graphs(data_name="dodoma", graph_name="last_graph") is used by all the plots, apart from the multiplot, to displaying the graphs in the output window

@Patowhiz
Copy link
Contributor Author

@Ivanluv thanks for the feedback, as discussed in you can take advantage of the newly added record_graph() function once PR #6060 is merged.

@Ivanluv
Copy link
Contributor

Ivanluv commented Apr 7, 2021

@Patowhiz I don't think its necessary to use record_graph() function here because the graphs are saved in this dialogue

@rdstern rdstern modified the milestones: 0.7.0, 0.7.3 May 26, 2021
@ChrisMarsh82 ChrisMarsh82 added this to To do in Team 1 via automation Oct 4, 2021
@ChrisMarsh82 ChrisMarsh82 moved this from To do to In progress in Team 1 Oct 4, 2021
@Ivanluv
Copy link
Contributor

Ivanluv commented Oct 14, 2021

@Patowhiz I don't think its necessary to use record_graph() function here because the graphs are saved in this dialogue

@Patowhiz what is your opinion

@shadrackkibet
Copy link
Collaborator

@Ivanluv could you check if there is anything we should address here otherwise we can close?

@Ivanluv
Copy link
Contributor

Ivanluv commented Oct 29, 2021

@shadrack from my end ,I don't think there's anything to be addressed

R-Instat Sprint March 2021 automation moved this from To do to Done Oct 29, 2021
Team 1 automation moved this from In progress to Done Oct 29, 2021
@ChrisMarsh82 ChrisMarsh82 removed this from Done in Team 1 Nov 15, 2021
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

No branches or pull requests

6 participants