search

natekupp / ffx

Fast Function Extraction
http://trent.st/ffx
Other
80 stars 97 forks source link

Second order interactions code is flawed #5

Closed javdrher closed 10 years ago

javdrher commented 11 years ago

The first part of the code which catches xi*xi: 1) order2_exponent is used nowhere 2) in the creation of combined_base var_i is used instead of basei.var, this variable is still alive from Step 1A and causes no error. However, only the last variable is added.

Furthermore, it seems more logic to me to include 2 in the list of allowed exponents, this way this part of the order-2 can just be removed completely.

In the second part, a break statement on len(order2_bases) >= max_n_order2_bases is added at the end of each for. However, only the inner for actually adds something to order2_bases and is able to violate the constraint. This means only the first one will ever be used.

jmmcd commented 11 years ago

Sorry for the delay. Patch welcome :)

Your comments make sense to me (with my limited knowledge of the code), except for the last. I think the goal is to break out of all loops when the limit is exceeded. When the inner one breaks, the outer two will break as well.

jmmcd commented 10 years ago

Furthermore, it seems more logic to me to include 2 in the list of allowed exponents, this way this part of the order-2 can just be removed completely.

I think it's not that simple -- FFX can use different "approaches", ie different types of primitives, and in any approach where the interactions are turned on, the exponents are turned off!