outsource autocogs to autocogs pkg

schloerke commented 6 years ago

use schloerke/autocogs pkg instead of internal auto cog fns

hafen commented 6 years ago

It's interesting that it passes CI. This code in the tests doesn't work for me installing it locally:

qplot(cty, hwy, data = mpg) +
  facet_trelliscope(~ class, self_contained = TRUE)

Error in function (x, ...)  : 
  Assertion on 'x' failed: Must be of type 'atomic scalar', not 'NULL'.

hafen commented 6 years ago

This example will run and print without errors, but there are no auto cogs:

library(gapminder)
library(ggplot2)
ggplot(gapminder, aes(year, lifeExp)) +
  geom_line() +
  facet_trelliscope(~ country + continent)

hafen commented 6 years ago

If I change geom_line() to geom_point(), that's what breaks the above example. I have ggplot2_2.2.1 installed (and so does TravisCI). So something's up with that and auto cogs are not being generated in the geom_line() case.

schloerke commented 6 years ago

"I can not debug your local cluster" 😂😂

There are not current auto cogs for a line. Should they be added?

The line has so many contexts. Should just the univariate_continuous be added?

On Tue, Sep 12, 2017 at 10:50 PM hafen notifications@github.com wrote:

It's interesting that it passes CI. This code in the tests doesn't work for me installing it locally:

qplot(cty, hwy, data = mpg) + facet_trelliscope(~ class, self_contained = TRUE)

Error in function (x, ...) : Assertion on 'x' failed: Must be of type 'atomic scalar', not 'NULL'.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/hafen/trelliscopejs/pull/47#issuecomment-329042287, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFsL2ufJY3Rm3DEbDWIkNsaWVF-_NV3ks5sh0LbgaJpZM4PQj6B .

hafen commented 6 years ago

Also we want to retain the behavior of the old facet_trelliscope() which computes summaries of any column not used in the plot. The way it did it before was to calculate the mean if it was numeric and varying within group, or just report the value if it was constant within a group. This allows users to provide their own user-defined cognostics in the facet_trelliscope() case by supplying custom columns to the input data.

hafen commented 6 years ago

Yeah maybe univariate_continuous should be added for line.

hafen commented 6 years ago

Thanks for this. I've merged this here https://github.com/hafen/trelliscopejs/tree/schloerke-autocogs with a couple of additional changes:

Got rid of the print() at the end of trelliscope() - we want to not print if the user does something like p <- trelliscope(...)
Set auto_cog = FALSE by default in facet_trelliscope(). We can make this an option where the user can specify for it to be true by default without specification, but given that it produces so many cognostics and I haven't added groups into the UI and also given the extra computation time, I think FALSE by default is better.

There are two other issues I'd like to work out before merging to master:

It would be nice for auto cogs to have descriptions that are different from the variables. For example, for the discrete univariate autocogs, instead of a cognostic description of "manufacturer_min_name" and "manufacturer_max_name", it would be nice to have a more meaningful description like "least prevalent manufacturer" and "most prevalent manufacturer", etc. I think we could come up with a template string for each type of autocog with a placeholder for the variable name, and then this gets populated as the desc argument to cog().
It appears that even with auto_cog = FALSE, auto cogs are still being computed. For example, try:

library(ggplot2)
qplot(cty, hwy, data = mpg) +
  facet_trelliscope(~ class, auto_cog = FALSE)

schloerke commented 6 years ago

I believe this is fixed now. descriptions were being stomped when binding rows.

qplot(cty, hwy, data = mpg[c("class", "cty", "hwy")]) + facet_trelliscope(~ class, auto_cog = FALSE)

qplot(cty, hwy, data = mpg[c("class", "cty", "hwy")]) + facet_trelliscope(~ class, auto_cog = TRUE)

hathawayj commented 6 years ago

When I try to install trelliscopejs it breaks on my students Mac with an error. It looks like autocogs requires java. Java wasn't a requirement previously. Since autocogs doesn't install it errors on the installation of trelliscopejs. Am I missing something on why Java is now needed?

hafen commented 6 years ago

autocogs uses the scagnostics package which depends on rJava. trelliscopejs has had this dependency for probably almost a year so it's strange that it is all of a sudden an issue. However, I've noticed rJava being more difficult on some linux systems lately and I really don't like having to depend on Java being configured correctly, so I just issued a PR that strips out most of scagnostics until we find a non-Java version.

schloerke commented 6 years ago

java in the scagnostics has now been removed

hafen / trelliscopejs

outsource autocogs to autocogs pkg #47