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

Contributing for an absolute beginner? #1273

Closed
mikeevmm opened this issue Mar 18, 2021 · 4 comments
Closed

Contributing for an absolute beginner? #1273

mikeevmm opened this issue Mar 18, 2021 · 4 comments

Comments

@mikeevmm
Copy link

Hello all,

I have recently implemented the bounded version of the L-BFGS-B in (a local copy of) Tensorflow Probability.

It's not a blazing fast implementation (it is outright hacky in some places --- e.g., how it calculates M for different histories), and I had to reduce scope in order to manage (I assumed the inputs were float32, and there was only one batching dimension; this fit my use case well). Despite this, L-BFGS-B is incredibly useful when employed in, e.g., periodic functions, and the code behaves correctly; it correctly converges for a high-dimensional quadratic surface as well as for a Rosenbrock surface (with a sufficiently large "box"), but converges to the same wrong minimum as the original Fortran implementation ((5., 5.)) when considering the Rosenbrock function and bounds [(5, 20), (-10, 5)].

I'd like to contribute this implementation to TensorFlow Probability, but I've read the CONTRIBUTING.md page (and related links) and am still at a lost. My main concerns are the following:

  1. Do I have to support arbitrary dtypes and arbitrary batching dimensions?
  2. Since I am not contributing a new module but would rather be putting the code into the optimizer module, how does this affect the recommendations?

It's also fairly difficult to grapple with all the information in CONTRIBUTING.md without being familiar with TensorFlow's internals. Is it possible to request a code review so I have something to go off of?

Thanks in advance

@davmre
Copy link
Contributor

davmre commented Mar 22, 2021

Hi Miguel, this sounds like a contribution we'd be happy to have (and those sound like good unit tests!). We do need to support arbitrary dtypes, and ideally multiple batch dimensions (though now that tf.vectorized_map exists it's becoming more feasible to work around this), but typically these are pretty straightforward and we have a fair amount of experience with idioms for making them work, so it's something we could cover in a code review if you're willing to iterate a bit. The best way to request review is to just create a new PR.

@mikeevmm
Copy link
Author

(Thank you @davmre for the prompt response; I haven't forgotten and will make a PR asap, but I'm fighting some deadlines right now :) )

@mikeevmm
Copy link
Author

mikeevmm commented Apr 4, 2021

Checking in, the pull-request has been filed ( #1290 ).

@mikeevmm
Copy link
Author

Since the pull request is moving along, I think this issue can be closed.

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

2 participants