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

Updating sensitivity analysis class to be general for any model parameters #426

Merged
merged 12 commits into from Apr 17, 2019

Conversation

Projects
None yet
3 participants
@JamesPino
Copy link
Contributor

commented Mar 22, 2019

The class was built for initial concentrations. This update allows any parameters to be used. Also added the ability to pass a list of parameter names rather than requiring all from the model to be used.

JamesPino added some commits Mar 22, 2019

Updating Sensitivity analysis class to be used with any parameters, n…
…ot just initial conditions. Also added ability to pass a list of parameter names rather than requiring all from the model to be used.
@coveralls

This comment has been minimized.

Copy link

commented Mar 22, 2019

Coverage Status

Coverage increased (+0.1%) to 78.954% when pulling ac7c427 on LoLab-VU:update_sensitivity_tool into b6ba27d on pysb:master.

@alubbock
Copy link
Member

left a comment

Nice, thanks @JamesPino. A few small suggestions:

  • Add an InitialsSensitivity wrapper to PairwiseSensitivity to avoid breaking API compatibility (example below)
  • Update the docstring to add the new sens_type and sample_list parameters
  • Add a quick example of parameter sensitivity sens_type="params" to the docstring examples section (which will also serve as a unit test)
  • Add other unit tests not covered by the above to test_sensitivity_analysis.py (e.g. the case where sens_type is None, and where sample_list is None - check the coverage report to see the areas of code not being tested).

Example compatibilty shim with deprecation warning (untested, but hopefully works):

class InitialsSensitivity(PairwiseSensitivity):
    def __init__(self, *args **kwargs)
        warnings.warn("InitialsSensitivity will be removed in a future "
                      "version of PySB. Use PairwiseSensitivity instead.",
                      DeprecationWarning,
                      stacklevel=2)
        super(InitialsSensitivity, self).__init__(*args, **kwargs)

JamesPino added some commits Mar 27, 2019

Adding examples in documentation for sens_type='params' and with a list
 of parameters. Adding tests for parameter list.
Removed a lot of the initializations of the solver. Not needed.
Added test to catch a non pysb simulator.
@JamesPino

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

I believe I address all the comments. The travis test is failing for python 3.6 due to an hash error during libroadrunner installation. It passed in the lolab-vu tests. I am unable to restart the tests for pysb/pysb.

@alubbock
Copy link
Member

left a comment

I've commented on a few small details. After resolving those I think we can merge. Thanks again!

Show resolved Hide resolved pysb/tests/test_sensitivity_analysis.py Outdated
Show resolved Hide resolved pysb/tests/test_sensitivity_analysis.py Outdated
Show resolved Hide resolved pysb/tools/sensitivity_analysis.py Outdated
Show resolved Hide resolved pysb/tools/sensitivity_analysis.py Outdated
Show resolved Hide resolved pysb/tools/sensitivity_analysis.py Outdated
Show resolved Hide resolved pysb/tools/sensitivity_analysis.py Outdated

JamesPino added some commits Apr 2, 2019

Changing doctest to verify size of array, rather than printing.
Windows represents np.array.shape as type L, where on linux it
uses it uses int. This is due to differences in C types. Pointless fix
in the long run, but now it should pass doctests on python2 for windows.
It is discussed here.
numpy/numpy#5809
@alubbock

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Thanks, just some small details remaining that I can see:

  • Add the sens_type parameter to the attributes list
  • Ideally, attributes which are also parameters should appear in the parameters list. Maybe make the parameters version contain the description, then write Identical to Parameters (see above) in the attributes section, as is done in a few places in core.py
  • Fix the Python 2.7 test on Windows (see Appveyor). Probably easiest to just remove the print(sens.b_matrix.shape) doctest (sorry, that was my suggestion - didn't realize it would break on Py27/Win!)
Adding attribute and parameter descriptions. I made some attributes
private that were supposed to be for internal calculations.
@JamesPino

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Thanks for the feedback. I fixed that silly python2 on windows error that day, just forgot to push. I added the attributes and parameters. Some of them should have been private, as they should not have been used elsewhere. I really want to rewrite this entire code from scratch. It was written by a younger me who tried to be clever when I didn't need to be.

@alubbock

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I really want to rewrite this entire code from scratch

That's often the way! When we make the move to PySB 2 there will be other backwards-incompatible changes, so if you want to make those changes in a separate branch I'd say go ahead; it can get merged in when the time comes.

If I'm being nitpicky, there's a stray O in your docstring: sens_type: {'params', 'initials', 'all'} Type of sensitivity analysis to perform. O. But otherwise, LGTM. Thanks again!

@alubbock alubbock merged commit 9d2d3bf into pysb:master Apr 17, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 78.954%
Details

@alubbock alubbock deleted the LoLab-VU:update_sensitivity_tool branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.