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

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

Closed
vanderwoodde opened this issue Mar 14, 2024 · 5 comments

Comments

@vanderwoodde
Copy link

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
Copy link
Owner

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
Copy link
Contributor

williamjamir commented May 3, 2024

@WillianFuks

I think this comment has a solution to the problem: tensorflow/probability#1799 (comment)

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 tensorflow/probability#1799 (comment).

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
Copy link
Owner

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
Copy link
Author

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

@WillianFuks
Copy link
Owner

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

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

3 participants