Ticket #2883 (inTicketWork defect)

Opened 6 years ago

Last modified 6 years ago

clarify scipy usage rules

Reported by: jbosch Owned by: robyn
Priority: normal Milestone:
Component: TCT Keywords:
Cc: robyn, jbecla, mjuric, ktl, rhl, smm, rowen, RayPlante, gpdf, mfreemon Blocked By:
Blocking: Project: LSST
Version Number:
How to repeat:

not applicable


I have been using scipy in some of my shapelet unit test code, with scipy an optional dependency and logic in the test code and build system to disable the particular tests that use scipy if it cannot be imported.

Robyn informs me that this is not an allowed usage of scipy according to the current rules, which state:

"Allowed only within ip_diffim and only if scipy eups available on system. For Developers' use only per RHL's strict constraint."

I would like to change this to:

"Allowed in test, example, and post-pipeline analysis code, but test usage must be guarded such that test code runs even when scipy is not present. May only be an optional dependency of any DM package. Usage within pipeline code is prohibited except as a temporary solution when expressly permitted by RHL."

(I believe the last sentence covers the current usage in ip_diffim, if it still exists)

My particular usage of scipy is to test my own implementation of Hermite polynomial evaluation against the one in scipy.special, and to test my shapelet-space convolution code against the convolution code in scipy.ndimage.

Change History

comment:1 Changed 6 years ago by ktl

IMHO, tests are nearly a first-class part of the code base. If we allow a package for anything but the most trivial tests, it should be considered required and allowed throughout.

comment:2 Changed 6 years ago by jbosch

In general, I agree. But I also think there's some utility to being able to test against a "reference implementation" even if it's not suitable for use in the pipeline as a whole or even something we want to demand be installable everywhere.

comment:3 Changed 6 years ago by ktl

Isn't it better to produce results from a reference implementation and then compare to those? At least the version of the reference would then be consistent. The result generator that uses scipy could be elsewhere (or in a tests/ subdirectory that is not executed).

I'm OK with scipy in examples. I'm a little leery of "post-pipeline analysis code".

comment:4 Changed 6 years ago by jbosch

That's a good point about saving the results from a reference implementation. I wouldn't have a problem with taking that approach in places where I'm currently using scipy for tests.

comment:5 Changed 6 years ago by robyn

From: Robert Lupton the Good ; Date: Wed, 5 Jun 2013 14:53:52 -0400

2.  I'm OK with Jim's:
> "Allowed in test, example, and post-pipeline analysis code, but test =
usage must be guarded such that test code runs even when scipy is not =
present. May only be an optional dependency of any DM package. Usage =
within pipeline codes prohibited except as a temporary solution when =
expressly permitted by RHL."

Once we can provide a binary distribution of python including =
numpy/scipy/matplotlib for os/x 10.7, 10.8, and all common Linux =
versions I'm happy to allow its use generally.

comment:6 Changed 6 years ago by robyn

  • Cc mfreemon added

This proposal was discussed at the TCT Meeting of 30 January 2014 attended by Russell Owen, K-T Lim, Robert Lupton, Mario Juric, and Jim Bosch. Mike Freemon sent his regrets since he had no opinion to offer on any of the proposals being reviewed. Gregory Dubois Felsmann sent his regrets but sent his proxy to K-T.

  • In light of DM's proposed move towards using of Anaconda Scientific Python distribution to provide integrated installation of many DM required science packages (e.g. numpy, matplotlib, scipy), this specific proposal is mostly irrelevant.
  • However, there were many considerations raised about using the scipy regardless of ease of installation and provenance.
  • Robert said that we should preferentially use numpy is the desired functionality is available there.
  • The question was raised if we needed to develop a list of scipy functionality approved for use in DM code
    • Someone said making a list for scipy, comparable to the approved boost functions, wasn't necessary since scipy is more unified than boost.
    • It was suggested that use of scipy should be something the Code Reviewer calls out for additional discussion to ensure it is an appropriate use. Using this method we could grow the list of approved scipy functionality as experience is gained.
    • K-T refined this into have 3 scipy usage lists: 1) preferred (aka Good), 2) not used (aka Bad), and (3) OK if necessary. To begin, all scipy functionality would be deemed OK and the Code Reviewers would start the process of differentiating functionality into the lists.
    • As a result of the preceding discussion it was stated that the expectation is that scipy should only be used if there is a compelling reason to use it. The Code Reviewer is expected to inquire about any scipy usage and the Code Reviewer will decide if that reason is compelling enough for its proposed use in the code under review.

K-T summarized the discussion so far:

  • Assuming the pre-condition that scipy becomes trivial to install:
    • In areas where no equivalent functionality in numpy exists, then scipy may be used.
    • If it is determined that a scipy functionality is not 'good' enough, then it is placed on the 'Bad' list.

K-T's summary was accepted.

The Chair noted that the migration to Anaconda python needed to be reviewed by the SAT and the SAT's recommendation then needed to be forwarded to the TCT. So until that time, this proposal is, presumably, moot.

comment:7 Changed 6 years ago by robyn

  • Status changed from new to inTicketWork

In advance of meeting the imposed constraint, the file: TCT/DmScipy was created and cited in the DM/ThirdPartySoftware. The file will be used to list scipy modules which have been reviewed and categorized: Acceptable or Banned.

THis Ticket will remain open until either Anaconda e or an alternate easily installable version of scipy is approved.

comment:8 Changed 6 years ago by robyn

  • Cc jbecla added
Note: See TracTickets for help on using tickets.