search

hiclib / pastis

Poisson-based algorithm for stable inference of DNA Structure
http://hiclib.github.io/pastis/
Other
34 stars 15 forks source link

Integrating diploid code: callbacks module #51

Closed gesinecauer closed 4 years ago

NelleV commented 4 years ago

I fixed the issue with travis. Can you please update your code with the latest master branch to include the fixes in your branch?

NelleV commented 4 years ago

Hi Gesine, As we discussed, I think the first step is to fix the failing tests. I would focus on this first before anything else: as long as the tests are not running, you may be introducing errors that you are not picking up.

We had discussed a while back about making many of the functions private: have you given any more thoughts to this? I really think you should provide as few functions as possible to the user and keep most of it either private or even locally on your computer: what is useful to you may not be useful to users. The 80:20 rule applies in software engineering: probably around 80% of the users will use only 20% of the code. You should aim at providing this 20% of the code (the rest becomes maintenance cost).

In terms of what remains to do before merging it:

I then have more specific comments on the code, but it would be easier to review if you reduce the amount of code and features provided in the pull request. I'm happy to pinpoint elements that are IMO not necessary to provide to the user (but that you can keep in a separate package for your use only).

gesinecauer commented 4 years ago

Hi Nelle, Yes, I have been focusing on resolving the bugs that are causing errors - I found quite a few yesterday but I haven't yet pushed the bug fixes to this repo. There's still a few more bugs I need to resolve as well.

Regarding private functions, I have not yet decided which functions to make private but I'll be working on that next. I actually do want to include all of the functions in this pull request in PASTIS (though many of them will be private, of course), and I can't currently think of anything that I'd be able to put in a separate package for my own use - I'll keep an eye out for this though, and feel free to make suggestions if you have any.

Aside from the 80:20 rule, do you have any suggestions for how to decide which functions will be private?

Thanks for your help!

gesinecauer commented 4 years ago

I fixed a bunch of bugs, but the tests are still failing. The errors I get when I run the tests on my machine are different from the errors I get with TravisCI, probably in part because I'm using python 3 and TravisCI is set up to use python 2?

The error I get when running on my machine is below. I don't understand this error at all, and I tried deleting __pycache__ and .pyc files but it didn't make a difference. __________________ ERROR collecting utils/tests/test_validation.py ___________________ import file mismatch: imported module 'test_validation' has this __file__ attribute: /net/noble/vol5/user/gesine/repos/pastis/pastis/externals/iced/utils/tests/test_validation.py which is not the same as the test file we want to collect: /net/noble/vol5/user/gesine/repos/pastis/pastis/utils/tests/test_validation.py HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

There are 2 errors with TravisCI... The first has to do with python2/python3 - TravisCI is upset that some of my print() functions have the keyword flush=True, which is only available in python 3. I can certainly switch to using sys.stdout.flush(), though I'm not sure this is worth it since we're going to be using python 3 as soon as the NB work is done, right? Let me know what you think.

The second error is confusing to me... I have no idea why config can't be imported here. ERROR: Failure: ImportError (cannot import name config) 863---------------------------------------------------------------------- 864Traceback (most recent call last): 865 File "/home/travis/miniconda2/envs/testenv/lib/python2.7/site-packages/nose/loader.py", line 407, in loadTestsFromName 866 module = resolve_name(addr.module) 867 File "/home/travis/miniconda2/envs/testenv/lib/python2.7/site-packages/nose/util.py", line 312, in resolve_name 868 module = __import__('.'.join(parts_copy)) 869 File "/home/travis/miniconda2/envs/testenv/lib/python2.7/site-packages/pastis/__init__.py", line 1, in <module> 870 from . import config 871ImportError: cannot import name config

gesinecauer commented 4 years ago

I just made a bunch of functions private. When you get the chance to look over the code, could you me know if you think any more functions should be private, or whether any of the private functions should actually be public?

NelleV commented 4 years ago

I've merged the PR #58 : I'm going to close this one. Can you do the tests and the documentation in two different branches? It should make reviewing easier.

Thanks