WillianFuks / tfcausalimpact

Python Causal Impact Implementation Based on Google's R Package. Built using TensorFlow Probability.
Apache License 2.0
593 stars 70 forks source link

ValueError: Argument `name` must be a string and cannot contain character `/`. Received: name=build_factored_surrogate_posterior/loc_0_momentum #86

Closed vanderwoodde closed 2 months ago

vanderwoodde commented 4 months ago

Hello,

When running the code from the example: `import pandas as pd from causalimpact import CausalImpact

data = pd.read_csv( 'https://raw.githubusercontent.com/WillianFuks/tfcausalimpact/master/tests/fixtures/arma_data.csv' )[['y', 'X']] data.iloc[70:, 0] += 5

pre_period = [0, 69] post_period = [70, 99]

ci = CausalImpact(data, pre_period, post_period) print(ci.summary()) print(ci.summary(output='report')) ci.plot()`

I received the following error: "ValueError: Argument name must be a string and cannot contain character /. Received: name=build_factored_surrogate_posterior/loc_0_momentum"

This only happens when using the default ('vi') fit method. If I update the fit_method to 'hmc' the code will run as expected.

My current package versions are: tensorflow: 2.16.1 tensorflow-probability: 0.24.0 tf-keras: 2.16.0 tfcausalimpact : 0.0.14

The tfcausalimpact package works as expected if I step back to: tensorflow: 2.15.1 tensorflow-probability: 0.23.0 tf-keras: 2.15.0

Would you recommend locking in those package versions if I am looking to run this in a production environment or will it be important to keep my tensorflow and causalimpact packages up to date?

WillianFuks commented 4 months ago

Hi @vanderwoodde ,

Just tested here and found the same issue. Unfortunately it doesn't seem there's much we could do on our side; apparently this is happening in the integration between keras and tfp which is outside of our control.

I opened an issue on their repo, let's see how the discussion evolves.

For now I recommend doing as you did, lowering versions specs might be good enough for now.

Thanks for letting us know about this, as soon as we find how to work around the issue I'll update tfci.

williamjamir commented 2 months ago

@WillianFuks

I think this comment has a solution to the problem: https://github.com/tensorflow/probability/issues/1799#issuecomment-2059955263

In my local system, I replaced this line https://github.com/WillianFuks/tfcausalimpact/blob/master/causalimpact/model.py#L362:

-optimizer = tf.optimizers.Adam(learning_rate=0.1)
+optimizer = tf_keras.optimizers.Adam(learning_rate=0.1)

After this, everything seemed to work fine. However, I am not sure whether there are any side effects since I am not familiar with the differences between tf_keras and keras.

Additionally, based on my understanding, tfp will not support keras 3. Here is the source https://github.com/tensorflow/probability/issues/1799#issuecomment-2077782271.

Edit: I just noticed the following warning (I'm using M2 CPU):

At this time, the v2.11+ optimizer `tf.keras.optimizers.Adam` runs slowly on M1/M2 Macs,
please use the legacy TF-Keras optimizer instead, located at `tf.keras.optimizers.legacy.Adam`.

Using

optimizer = tf_keras.optimizers.legacy.Adam(learning_rate=0.1)

I was able to make the warning message disappear, which slightly sped up the execution time. Perhaps the final solution could check for the arm64 arch 😄

import platform
import tf_keras

keras_optimizer = tf_keras.optimizer.legacy if platform.machine() == 'arm64' else tf_keras.optimizer
optimizer = keras_optimizer.Adam(learning_rate=0.1)
WillianFuks commented 2 months ago

Thanks for the input @williamjamir , I just implemented as you suggested, things seems to be working now.

Please @vanderwoodde , could you test the rc with the fix? just run pip install tfcausalimpact==0.0.15rc0 and this issue should be solved. If possible please let me know if it's working for you now; if it is then I'll merge the branch to master.

vanderwoodde commented 2 months ago

@WillianFuks Yes, that rc solved the issue I was having. The sample code ran error-free with 0.0.15rc0. Thanks!

WillianFuks commented 2 months ago

Thanks for the update. Stable version has just been released: pip install -U tfcausalimpact==0.0.15