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

greta silently ignoring base functions #317

Closed
Ilia-Kosenkov opened this issue Sep 3, 2019 · 9 comments · Fixed by #330
Closed

greta silently ignoring base functions #317

Ilia-Kosenkov opened this issue Sep 3, 2019 · 9 comments · Fixed by #330

Comments

@Ilia-Kosenkov
Copy link

I was recently debugging an issue with my simulations, where the optimal parameters obtained using greta were not actually optimal, so model data, reconstructed using greta output, were systematically offset from the real data. Having checked everything multiple times, I turned to the output of print(plot(greta_model)) with the help of DiagrammeR. Turned out, greta silently dropped calls to log10 function while constructing the model. Check the simplest reprex and the output of the DiagrammeR as I get on my machine.

I am not sure what causes this, because greta successfully parsed a complex model containing calls to custom R functions that perform numerical computations, including exp , ^ and base arithmetics, yet log10 was dropped.

Providing that greta models are defined using R code and support user-defined functions (e.g. if I define lg <- funciton(x) log(x) / log(10), it will be unrolled into two operations), I find it surprising that logs are unsupported (without error or warning).

Param <- uniform(0, 10)
A <- log10(Param + 1)
B <- log(Param + 1)
greta_model <- model(Param)
# Draw model using DiagrammeR & export to png

output

@goldingn
Copy link
Member

goldingn commented Sep 3, 2019

Wow, that is weird!

I would have expected log10() to error here, since there isn't a log10 function implemented for greta arrays. I'm away from my laptop bit will try to dig into this soon.

It would be very easy to add a log10 function to greta to resolve this specific situation. But I'd like to understand why this function would silently return its input unevaluated.

@goldingn
Copy link
Member

goldingn commented Sep 3, 2019

log10() is a primitive. Primitive functions dispatch differently than other R functions so I suspect this weird interaction may only happen with mathematical operations that are primitives, and for which no greta array version is defined.

The following also fall in that category, so should be checked: log2, cosh, sinh, tanh, acosh, asinh, atanh, cospi, sinpi, tanpi, gamma, trigamma, cummax, cummin, Im, Re, Arg, Conj, Mod

@jeffreypullin
Copy link
Contributor

Yes, I just noticed that. The source for log10 and log2 is here. But it's really a wrapper around math2 here. It's certainly not apparent why this weird interaction is taking place looking at the code though.

Also note that:

x <- as_data(10)
log10(x)

Also shows the weird return the argument behaviour.

@Ilia-Kosenkov
Copy link
Author

Maybe you can still add some warning/message if a function cannot be exported correctly? I would prefer a solid error to prevent compiling a completely different than specified model.

@jeffreypullin
Copy link
Contributor

Consider:

x <- as_data(10)
log10(x) # returns the argument (or at least appears to)

x <- unclass(x)
log10(x) # works sort of - changes the data in the array but not the node data

So I think the issue is that the primitives ignore the node attribute and work on the value stored in the greta array. So it then appears that the they have no effect as the node value is what is exposed to the user.

@Ilia-Kosenkov
Copy link
Author

That is indeed the behaviour that I experienced in my real-life application - log10 calls were just ignored and data were transported to the next layer without change.

@goldingn
Copy link
Member

goldingn commented Sep 3, 2019

Right. It would be good if we could error nicely if a greta array is passed to some arbitrary function that doesn't support it.

Unfortunately, erroring is the function's responsibility, there's not much the object can do to trigger an error. Usually a function will error with a message about not being able to work with a greta array. But apparently not here!

By the way, you can see the list of functions greta supports here: https://greta-stats.org/reference/functions.html

@jeffreypullin
Copy link
Contributor

I think the 'issue' is the following behaviour:

test <- 10
class(test) <- "test"
log10(test)
#> [1] 1
#> attr(,"class")
#> [1] "test"

Created on 2019-09-04 by the reprex package (v0.2.1)

i.e. that numeric functions ignore the attributes of an object and operate directly on the 'data'

Because greta_arrays are really just R array's with a node attribute the numeric functions (at least) just operate on their array data - which appears to the user as if the functions are having no effect.

Two ideas to fix this:

  1. Add methods for all possible numeric functions - which could just throw errors initially
  2. Convert the 'data' of each greta array to a list - this would lead to mathematical functions throwing a 'non-numeric' error (this may have other consequences I haven't considered though)

All the best!

@goldingn
Copy link
Member

goldingn commented Sep 4, 2019

Thanks for investigating @jeffreypullin!

greta arrays actually used to be lists, but were changed to be arrays fairly recently. In part because other behaviour of having them be lists was problematic (I think concatenation with NULL was one issue).

I think a more thorough coverage of numeric functions would probably be the easiest and best option. Most of them will be in TensorFlow anyway, so would only take a couple of lines to implement.

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 a pull request may close this issue.

3 participants