Hacker News Reader

Comment by garmaine | Hacker News Reader

I’m very sorry to see someone who obviously cares so much to be defending this code. This does not follow best practices, and using complexity of the underlying algorithm is just an excuse. Complex code can be beautiful and well documented.

Writing a complex method in 174 lines is not elegant nor beautiful. Writing a well documented file that can take an engineer in a different specialty and bring them up to speed in 1,000 lines is.

View on HN

No matter how much you comment your code, you are not going to bring people up to speed on an algorithm that requires background knowledge on dozens of scientific papers, hundreds of pages, with a few code comments. This code is aimed at researchers who are familiar with the techniques and have the necessary background knowledge. For such people, the code is very readable.

Think about it like this: If you write a game engine, are you going to document each function with proofs that explain how the underlying Physics works for people who don't have such knowledge? No, you assume that people who read game engine code have read some basic physics books.

We also have code like that. For example, that's the explicit goal of the Spinning Up repo: https://github.com/openai/spinningup/blob/master/spinup/algo...

In practice, it's much harder to use that code, and we tend not to consume code like that internally. There's a real tradeoff!

Well then I’m sorry for that. It’s a good indicator of a broken culture.

ddpg() takes 17 parameters and is over 200 lines long. I'm very far from being a domain expert, but having worked in other complex domains, I'm pretty confident this can be redesigned such that it's both more maintainable and more pleasant to use.

Hello! Spinning Up author here. I would love to hear your thoughts on this! So far I have had a lot of success teaching people about DDPG using this code example, but I'm grateful for every fresh perspective. :)

Feel free to reach out by email, jachiam at openai.

There is no function in the world that should ever take 17 parameters. If the algorithm permits such configuration, as I am sure it does, then it should take a configuration object which has all these values. The object could then be constructed using special purpose factories that take fewer parameters, and then customized from there as needed.

It may be an indication that the whole thing needs refactoring though.

You refactor that way but then you make it unnecessarily more complicated.

Reply on news.ycombinator.com