Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not fail when bcolz import fails #161

Merged
merged 6 commits into from May 13, 2015
Merged

Do not fail when bcolz import fails #161

merged 6 commits into from May 13, 2015

Conversation

@benjello
Copy link
Collaborator

@benjello benjello commented May 12, 2015

No description provided.

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 12, 2015

Code Health
Code quality remained the same when pulling e74381f on benjello:master into 02b807d on liam2:master.

@gdementen
Copy link
Member

@gdementen gdementen commented May 12, 2015

If you want to make bcolz optional (which is fine with me given its very limited usage), please include an explicit exception if its functionality (interpolate in this case) is used anyway.

2 other places need to be adapted too:

@benjello
Copy link
Collaborator Author

@benjello benjello commented May 13, 2015

Just starting the conversation to test your mood ;-)
More commits to come. Please do not hesitate to be straightforward in your comments, it actually helps me understand your priorities.

@benjello
Copy link
Collaborator Author

@benjello benjello commented May 13, 2015

Could you write the sentence you would like to appear in https://github.com/liam2/liam2/blob/master/INSTALL ?

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 13, 2015

Code Health
Code quality remained the same when pulling cf9ff0e on benjello:master into 02b807d on liam2:master.

@gdementen
Copy link
Member

@gdementen gdementen commented May 13, 2015

just move it to the optional dependencies paragraph with something like:

to import data with interpolation/data with missing data points (eg several time series with different dates for the same individual):

src/main.py Outdated
try:
import bcolz
except ImportError:
bcolz = None

This comment has been minimized.

@gdementen

gdementen May 13, 2015
Member

plz move this below the "optional deps" comment

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 13, 2015

Code Health
Code quality remained the same when pulling 92aefda on benjello:master into 02b807d on liam2:master.

src/main.py Outdated
@@ -210,7 +213,7 @@ def __call__(self, parser, namespace, values, option_string=None):
matplotlib {mpl}
""".format(py=py_version, np=numpy.__version__, ne=numexpr.__version__,
pt=tables.__version__, vt=vt_version, mpl=mpl_version,
bc=bcolz.__version__, yml=yaml.__version__))
bc=bcolz.__version__ if bcolz is not None else None, yml=yaml.__version__))

This comment has been minimized.

@gdementen

gdementen May 13, 2015
Member

plz use N/A when missing, and use the same template (bc_version = ...) than for other optional deps (it is not especially better, but lets be consistent)

@@ -143,5 +143,12 @@ def vitables_data_files():
ext_modules=ext_modules,
options={"build_ext": build_ext_options, "build_exe": build_exe_options},
executables=[Executable("main.py")],
requires=['numpy', 'numexpr', 'tables', 'bcolz'])
install_requires=['numpy', 'numexpr', 'tables'],
extra_requires=dict(

This comment has been minimized.

@gdementen

gdementen May 13, 2015
Member

Thanks for this! I never bothered to look up the exact syntax :)

This comment has been minimized.

@benjello

benjello May 13, 2015
Author Collaborator

Not tested though !

This comment has been minimized.

@gdementen

gdementen May 13, 2015
Member

Please test then ;-)

On Wed, May 13, 2015 at 10:51 AM, Mahdi Ben Jelloul <
notifications@github.com> wrote:

In src/setup.py
#161 (comment):

@@ -143,5 +143,12 @@ def vitables_data_files():
ext_modules=ext_modules,
options={"build_ext": build_ext_options, "build_exe": build_exe_options},
executables=[Executable("main.py")],

  •  requires=['numpy', 'numexpr', 'tables', 'bcolz'])
    
  •  install_requires=['numpy', 'numexpr', 'tables'],
    
  •  extra_requires=dict(
    

Not tested though !

Reply to this email directly or view it on GitHub
https://github.com/liam2/liam2/pull/161/files#r30212604.

This comment has been minimized.

@benjello

benjello May 13, 2015
Author Collaborator

I actually use another setup.py hacked to work on a linux machine so won't be able to test it for windows users (@AlexisEidelman ?). I do not plan to submit a complete overhaul of setup.py (with a travis CI config file etc) this early. My pint with this commit was to propagate upstream as soon as possible what could be propagated without to much hassle.
But i did checked the syntax though ;-)

This comment has been minimized.

@gdementen

gdementen May 13, 2015
Member

hmmm a quick google seem to suggest "extras_require" instead of "extra_requires"...

This comment has been minimized.

@benjello

benjello May 13, 2015
Author Collaborator

Sorry I will fix that

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 13, 2015

Code Health
Code quality remained the same when pulling 55edc2b on benjello:master into 02b807d on liam2:master.

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 13, 2015

Code Health
Code quality remained the same when pulling fd80452 on benjello:master into 02b807d on liam2:master.

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 13, 2015

Code Health
Code quality remained the same when pulling 1895ae8 on benjello:master into 02b807d on liam2:master.

@gdementen
Copy link
Member

@gdementen gdementen commented May 13, 2015

Looks good to me, thanks!

gdementen added a commit that referenced this pull request May 13, 2015
Do not fail when bcolz import fails
@gdementen gdementen merged commit 449128f into liam2:master May 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.