google-research / torchsde

Differentiable SDE solvers with GPU support and efficient sensitivity analysis.
Apache License 2.0
1.52k stars 195 forks source link

Add falling back imports for brownian_lib #50

Closed dboyliao closed 3 years ago

dboyliao commented 3 years ago

Add falling back import statements for brownian_lib module. Or tests may fail to run due to AttributeError or ImportError

googlebot commented 3 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

dboyliao commented 3 years ago
Screen Shot 2020-09-07 at 4 27 26 PM
dboyliao commented 3 years ago

@googlebot I fixed it.

patrick-kidger commented 3 years ago

Googlebot doesn't seem convinced! Did you enter your GitHub user name when you signed the CLA?

In any case - this is only an issue for running the tests, correct? In which case I'd suggest that probably the better thing to do would be to modify the tests to skip brownian_lib if it doesn't exist. (Also I'd suggest working from the dev branch rather than the master branch.)

lxuechen commented 3 years ago

So the folder brownian_lib is to explicitly differentiate between the Python implementation and C++ implementation.

It doesn't really make sense to let brownian_lib import the same classes. I'd probably modify the tests with something like

if not hasattr(torchsde.brownian_lib, "BrownianPath"):
...

or

try:
    torchsde.brownian_lib.BrownianPath
except AttributeError:
...

Also, I appreciate the effort in sending in a PR, but I'd recommend opening up an issue first. It doesn't quite make sense to send in a PR before initiating any type of discussion.

dboyliao commented 3 years ago

@patrick-kidger Yes, I managed to get tests work with adding imports. I inspect the code of the tests and find out that BrownianPath and BrownianTree from both modules should work compatibly. That's why I come up with fix. I check the link googlebot gave me and it looks like I did sign the CLA before. Not sure why it does not work.

@lxuechen You are right, I should go for issue tracker first. Sorry for rushing an PR without discussion. Will close this one and open up an issue instead. Thanks for your suggestions.