[yt-dev] Analysis module review process

John ZuHone jzuhone at gmail.com
Wed Dec 30 14:39:07 PST 2015


It seems to me that something probably ought to be done, because most of the analysis modules are so domain-specific that they often require expertise in that particular area in order to give a proper review beyond just code errors and/or style. This is often what happens, in PR hangouts I often hear “well, the code itself looks good, even though I have no idea what it does."

photon_simulator is an example of this (albeit an extreme one). I’ve contemplated spinning it off into a separate package, given that its size and complexity are both getting rather large, but given that we have more than a handful of users for it I feel that this would be too disruptive.

As an example of how to do “affiliated packages”, AstroPy has such a notion, but I always found the process by which a package achieves this status rather complicated. 

http://www.astropy.org/affiliated/ <http://www.astropy.org/affiliated/>

> On Dec 30, 2015, at 5:25 PM, Matthew Turk <matthewturk at gmail.com> wrote:
> 
> Hi all,
> 
> This is an interesting discussion ... and to be honest, I'm kind of
> left wondering if maybe we should be encouraging more analysis modules
> to be external packages.  I came into this thinking that we would want
> to make analysis modules behave more like external packages that get
> bundled, but I'm not sure that's the case after all.  Unfortunately,
> we don't *have* a method for defining "affiliated" packages, and we
> certainly don't have (or *want*) an update mechanism for external
> packages that build on yt.  But this becomes something of a
> self-reinforcing loop ... Maybe we should come up with something like
> that, so that projects that build on yt can be "advertised" somehow in
> the base package, or something.  I am not sure.
> 
> Anyway, the responses to this thread make me think perhaps we
> shouldn't lower the review barrier after all -- going from 3 to 2 is
> perhaps just window dressing, and maybe we should encourage the
> analysis modules to have higher bus factors.
> 
> -Matt
> 
> On Tue, Dec 29, 2015 at 5:09 PM, Nathan Goldbaum <nathan12343 at gmail.com> wrote:
>> I agree. If we can identify maintainers for analysis modules, I think only
>> one additional approval besides the maintainer is fine. If the maintainer
>> proposes the PR, they should try to get at least one person to look over it
>> in detail before it gets merged.
>> 
>> On Tuesday, December 29, 2015, Britton Smith <brittonsmith at gmail.com> wrote:
>>> 
>>> I agree that the number of approvals is less important than ensuring that
>>> PRs come with appropriate tests and docs.  If the PR is a change to an
>>> existing analysis module, we should just make sure the relevant devs are
>>> identified and that they sign off on it.  If it's something brand new, I
>>> think 2 or 3 people is probably still a good idea.
>>> 
>>> On Tue, Dec 29, 2015 at 4:44 PM, Matthew Turk <matthewturk at gmail.com>
>>> wrote:
>>>> 
>>>> Ideally, with the barrier to acceptance reduced, the reviews that come
>>>> in may be deeper and more thoughtful ... and less along the lines of
>>>> "Can somebody just hit the approve button?"
>>>> 
>>>> On Tue, Dec 29, 2015 at 4:41 PM, John ZuHone <jzuhone at gmail.com> wrote:
>>>>> +1 for lowering the barrier. At a minimum, tests should pass, and new
>>>>> tests
>>>>> should be encouraged if possible. Docs as well.
>>>>> 
>>>>> Not too worried about 1 or 2 approvals. We could try 2 at first to see
>>>>> if
>>>>> this helps things out.
>>>>> 
>>>>> On Dec 29, 2015, at 5:39 PM, Cameron Hummels <chummels at gmail.com>
>>>>> wrote:
>>>>> 
>>>>> As long as the code being changed is local to an analysis module and
>>>>> not
>>>>> used by other parts of the main yt codebase, yes, I'm all for dropping
>>>>> the 3
>>>>> reviewer requirement to 2.  1 might be pushing it though.
>>>>> 
>>>>> On Tue, Dec 29, 2015 at 2:38 PM, Matthew Turk <matthewturk at gmail.com>
>>>>> wrote:
>>>>>> 
>>>>>> That's precisely what I had in mind.
>>>>>> 
>>>>>> On Tue, Dec 29, 2015 at 4:36 PM, Nathan Goldbaum
>>>>>> <nathan12343 at gmail.com>
>>>>>> wrote:
>>>>>>> On Tue, Dec 29, 2015 at 4:29 PM, Matthew Turk
>>>>>>> <matthewturk at gmail.com>
>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> [+-][01] on reducing review overhead for analysis modules?
>>>>>>> 
>>>>>>> 
>>>>>>> Does this just mean reducing the number of PR reviewers before we
>>>>>>> merge
>>>>>>> pull
>>>>>>> requests? I'd be ok with that, but just want to clarify what you
>>>>>>> have in
>>>>>>> mind.
>>>>>>> 
>>>>>>> _______________________________________________
>>>>>>> yt-dev mailing list
>>>>>>> yt-dev at lists.spacepope.org
>>>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>>>> 
>>>>>> _______________________________________________
>>>>>> yt-dev mailing list
>>>>>> yt-dev at lists.spacepope.org
>>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> --
>>>>> Cameron Hummels
>>>>> NSF Postdoctoral Fellow
>>>>> Department of Astronomy
>>>>> California Institute of Technology
>>>>> http://chummels.org
>>>>> _______________________________________________
>>>>> yt-dev mailing list
>>>>> yt-dev at lists.spacepope.org
>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>> 
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> yt-dev mailing list
>>>>> yt-dev at lists.spacepope.org
>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>> 
>>>> _______________________________________________
>>>> yt-dev mailing list
>>>> yt-dev at lists.spacepope.org
>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>> 
>>> 
>> 
>> _______________________________________________
>> yt-dev mailing list
>> yt-dev at lists.spacepope.org
>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>> 
> _______________________________________________
> yt-dev mailing list
> yt-dev at lists.spacepope.org
> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.spacepope.org/pipermail/yt-dev-spacepope.org/attachments/20151230/c451fbd0/attachment.htm>
-------------- next part --------------
_______________________________________________
yt-dev mailing list
yt-dev at lists.spacepope.org
http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org


More information about the yt-dev mailing list