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

Shiny GSoC Final Submission #1594

Merged
merged 13 commits into from Aug 26, 2017

Conversation

shubhamagarwal92
Copy link
Contributor

This PR is related to the final submission for "Scientific Visualization" project GSoC 2017.

Description

Major changes for the project:

  1. Adding interactiveness using ggploltly
  2. Refactoring of code
  3. Load multiple outputs of the model
  4. Ways to toggle geometries (e.g. geom_point vs. geom_line).
  5. Smoothing using geom_smooth (Slider for specifying moving window width)
  6. Comparing model output vs loaded data

Information related to previous related PRs and commit history can be found here

Motivation and Context

These changes are related to the required enhancements in the existing workflowPlot Shiny app.

Types of changes

  • [ x] Bug fix (non-breaking change which fixes an issue)
  • [ x] New feature (non-breaking change which adds functionality)
  • [ x] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the CHANGELOG.md.
  • [ x] I have updated the documentation accordingly.
  • [ x] I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ashiklom ashiklom self-requested a review August 22, 2017 14:29
Copy link
Member

@ashiklom ashiklom left a comment

Choose a reason for hiding this comment

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

In general, looks good, except for the following:

  • The merge conflict needs to be resolved
  • The ui.R script is missing call to library(plotly) for the plotlyOutput function. I just discovered this while testing on our servers.
  • Additional cleanup, described below.

# We can also save the csv on the run from the shiny app as well
# write.csv(inputs_df,file='/home/carya/pecan/shiny/workflowPlot/inputs_df.csv',
# quote = FALSE,sep = ',',col.names = TRUE,row.names=FALSE)

Copy link
Member

Choose a reason for hiding this comment

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

Commented code should be removed.

# file format can be retrieved using either by input or format id.
getFileFormat <- function(bety,input.id,format.id=NULL){
# Retaining the code for getting file format using format Id as in tutorial
# TODO Retaining the code for getting file format using format Id as in tutorial
Copy link
Member

Choose a reason for hiding this comment

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

This function is not necessary, since it just directly calls query.format.vars with the same arguments. You should remove it here and replace it elsewhere with the direct call.

configPath <- file.path(basePath, 'pecan.CONFIGS.xml')
# Second way of proving configPath. More of a hack
# Second way of providing configPath. More of a hack
Copy link
Member

Choose a reason for hiding this comment

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

Remove this and the following line.

# File_format <- PEcAn.DB::query.format.vars(bety = bety, format.id = format.id)
File_format <- PEcAn.DB::query.format.vars(bety = bety, input.id = input.id)
return(File_format)
}
getSettingsFromWorkflowId <- function(bety,workflowID){
basePath <- tbl(bety, 'workflows') %>% filter(id %in% workflowID) %>% pull(folder)
basePath <- tbl(bety, 'workflows') %>% dplyr::filter(id %in% workflowID) %>% pull(folder)
Copy link
Member

Choose a reason for hiding this comment

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

dplyr::tbl, dplyr::pull

return(inputIds)
# Subsetting the input id list based on the current (VM) machine
my_hostname <- PEcAn.utils::fqdn()
my_machine_id <- tbl(bety, 'machines') %>% dplyr::filter(hostname == my_hostname) %>% pull(id)
Copy link
Member

Choose a reason for hiding this comment

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

dplyr::tbl, dplyr::pull

inputs_df <- tbl(bety, 'dbfiles') %>%
dplyr::filter(container_type == 'Input', machine_id == my_machine_id) %>%
inner_join(tbl(bety, 'inputs') %>% dplyr::filter(site_id %in% site_Id), by = c('container_id' = 'id')) %>%
collect()
Copy link
Member

Choose a reason for hiding this comment

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

dplyr::inner_join, dplyr::collect

@ashiklom
Copy link
Member

Also, in the get_workflow_ids function in PEcAn.db, you should set ensembles = FALSE. This dramatically increases the speed of the query, and returns more workflows (with ensembles = TRUE, you only get workflows that had an ensemble step).

@robkooper
Copy link
Member

@shubhamagarwal92 can you update the changelog as well to indicate this new functionality

@mdietze mdietze merged commit 7c1fbd5 into PecanProject:develop Aug 26, 2017
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.

None yet

4 participants