Builds failing

[arrrobase]

Builds are failing in both OSX and Linux because of import errors trying to import nems_lbhb in nems/plots/quickplot.py:26. Do we want nems_db to be installed as well, or is the idea that nems should be able to pass tests without nems_db?

From TravisCI:

_____________________ ERROR collecting tests/test_plots.py _____________________
ImportError while importing test module '/home/travis/build/LBHB/NEMS/tests/test_plots.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/test_plots.py:8: in <module>
    import nems.plots.api as nplt
nems/plots/api.py:20: in <module>
    from .quickplot import quickplot, before_and_after_scatter
nems/plots/quickplot.py:26: in <module>
    from nems_lbhb.gcmodel.guiplots import (contrast_kernel_heatmap,
E   ModuleNotFoundError: No module named 'nems_lbhb'
____________________ ERROR collecting tests/test_xforms.py _____________________
ImportError while importing test module '/home/travis/build/LBHB/NEMS/tests/test_xforms.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/test_xforms.py:5: in <module>
    import nems.xforms as xforms
nems/xforms.py:23: in <module>
    import nems.plots.api as nplt
nems/plots/api.py:20: in <module>
    from .quickplot import quickplot, before_and_after_scatter
nems/plots/quickplot.py:26: in <module>
    from nems_lbhb.gcmodel.guiplots import (contrast_kernel_heatmap,
E   ModuleNotFoundError: No module named 'nems_lbhb'

[svdavid]

@jacobpennington At some point we need to revamp quickplot. In the short term though, we should figure out a way to avoid dependencies on nems_db in nems.

[jacobpennington]

@svdavid I think the simplest thing would be to just stop using quickplot altogether and use the .plot method that you built into the Modelspec object. That way plot functions from nems_db are only imported if the specified modelspec needs them, and then it's the user's responsibility to only ask for those plots if they have the extra package installed.

Downside is that currently the plot method is less flexible than quickplot.

[svdavid]

@jacobpennington I agree, and I already mentioned it to @awctomlinson . There's already a rudimentary plotter in place. It just doesn't have a couple convenient features that are hard-coded into the current quickplot. Since we're starting to expand the scope of models being tested (eg, charlie's population models) this is a good time to think about that plotting function.

[arrrobase]

Another option, until modelspec.plot is up and running, is to remove the nems_lbhb import statement from quickplot. It seems to only be used for if keywords containing the following text:

• contrast_kernel
• contrast
• summed_contrast_kernel

Since quickplot is part of nems and not nems_db, it makes sense it should only support the keywords that come from nems and not nems_db. I couldn't find any plugins included with nems that mention contrast.

[jacobpennington]

The contrast keywords are for a specific model that i've been using, so removing the import from quickplot would mean that the default figure couldn't be generated for those models. So yes, the import needs to be removed but not until the functionality of quickplot can be replaced - it was put in as a temporary kludge until that problem gets solved. We could move all of the relevant contrast code from nems_lbhb into nems, but that would defeat the point of having a separate repository for lab-specific models. Maybe it would make sense to just include the contrast model in the main repo, but there will inevitably be cases where the model is niche enough that it's not worth including.

Long-term we need a plotting routine that can happily point to functions outside of the main nems repo at runtime without having to import them in the code.

[svdavid]

Those functions should be visible in the plot_fn list associated with each module. That's how modelspec.quickplot works. A quick fix would be to use the default plot_fn, thus only importing from nems_lbhb if the module asks for it. stephen

On Wed, Dec 4, 2019 at 1:35 PM jacobpennington notifications@github.com wrote:

The contrast keywords are for a specific model that i've been using, so removing the import from quickplot would mean that the default figure couldn't be generated for those models unless we replace the functionality of quickplot with something else. We could move all of the relevant contrast code from nems_lbhb into nems, but that would defeat the point of having a separate repository for lab-specific models. Maybe it would make sense to just include the contrast model in the main repo, but there will inevitably be cases where the model is niche enough that it's not worth including.

Long-term we need a plotting routine that can happily point to functions outside of the main nems repo at runtime without having to import them in the code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/LBHB/NEMS/issues/160?email_source=notifications&email_token=AB77ZJZULAVG25PELJTYZWTQXAPC3A5CNFSM4JU77AM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF6SK5A#issuecomment-561849716, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB77ZJYZ4HG73MELD2Y76G3QXAPC3ANCNFSM4JU77AMQ .

[jacobpennington]

Right, i'm referring to nems.plots.quickplot. Changing the default behavior to use modelspec.quickplot would work fine for me.