[SciPy-Dev] pull requests and code review

josef.pktd@gmai... josef.pktd@gmai...
Thu Jan 5 18:55:15 CST 2012


On Thu, Jan 5, 2012 at 7:41 PM, Travis Oliphant <travis@continuum.io> wrote:
> I don't think there should be any time limit on pull requests.     Who is the *we* that needs time to look at them.   I did take time to look at the changes.   The code changes were not extensive (except for the very nice tests), and it is a welcome change.

We is whoever is interested and might be affected by the changes. I'm
looking at all stats pull request and most or all pull requests that
will have a direct impact on statsmodels.

It takes some time until I see the notification that a pull request
has been opened and some time to see whether it might be a change that
affects statsmodels.

As soon as a pull request is closed github doesn't send out any
notifications anymore. So the discussion is closed, except for
developers that are already participating in the pull request. At
least that is my impression of github from the past experience.


>
> Your feedback on the use of inspect is very good.   We can take a look at whether or not it method calls were considered and fix it if it does.

relying on inspect is very fragile, and it depends a lot on the
details of the implementation, so I'm always sceptical when it's used.
In this case it compares the hessian signature with the signature of
the main function, if they agree then everything is fine. But I'm not
sure it really works in all our (statsmodels) use cases.

>
> If you are interested in continuing to review all the optimize changes, I will make sure and give you time to review in the future.    This is where having a list of interested and available parties for different modules would make a great deal of sense.

Since statsmodesl is a very heavy user of scipy.optimize, I'm keeping
almost complete track of any changes.

Josef

>
> Thanks,
>
> -Travis
>
>
> On Jan 5, 2012, at 6:19 PM, josef.pktd@gmail.com wrote:
>
>> Can we keep pull requests open for more than 3 hours, so we actually
>> have time to look at them.
>>
>> looking at
>> https://github.com/scipy/scipy/commit/c9c2d66701583984589c88c08c478e2fc6d9f4ec#L1R1213
>>
>> my first guess is that the use of inspect.getargspec breaks when the
>> hessian is a method attached to a class, as we have almost all our
>> code in statsmodels.
>> We just fixed a similar case in scipy.
>>
>> There should be at least time to check whether this kind of suspicions
>> are justified or not.
>>
>> Josef
>> _______________________________________________
>> SciPy-Dev mailing list
>> SciPy-Dev@scipy.org
>> http://mail.scipy.org/mailman/listinfo/scipy-dev
>
> _______________________________________________
> SciPy-Dev mailing list
> SciPy-Dev@scipy.org
> http://mail.scipy.org/mailman/listinfo/scipy-dev


More information about the SciPy-Dev mailing list