You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This fork started with me not being able to run sensitivity analyses inside my OMEdit client because of a lookup issue inside the varAnalysisForPerturbedParam function. In my case the float 0.1000000001 was compared against 0.1 resulting in a key error. I fixed this by looking up the nearest datapoint to the query. There were similar issues with the rootMeanSquareForVar function. Both of these are inside indiv_sens.py and are the only change that can be noticed from the user side.
Every other change just adds typehints that make working with the code a lot more managable. I verified types by purposfully raising exceptions, then printing the type inside the error message. While doing this I noticed a lot of dead code, meaning classes that never get called during the analysis (from inside OMEdit), which are bloating up the module quite a bit.
I work with Python professionally and am eager to contribute more to this tool, as I see a lot of value in it. I'd also love to add more functionality in the future that I personally use in the field (i.e. an FFT Tool, Campbell plots, etc.).
(I couldn't get the tests to actually run, but I tested the 2 changes with live models inside OMEdit)
Related Issues
I couldn't find an issue relating to the key error crash yet
Thanks for detecting these issues and providing the candidate fixes.
From a numerical and software-engineering perspective, the argument presented in the PR is coherent.
Sensitivity analyses typically assume continuous or quasi-continuous parameter spaces, in which selecting the nearest available sampled value is often the correct semantic interpretation.
The fix is sound provided that:
-the independent variable is monotonic or otherwise ordered;
-the sampling grid is sufficiently dense; and
-the distance metric used to define “nearest” (absolute vs. relative) is well defined.
The current implementation does not appear to use an explicit tolerance (eps) or a guard against large distance mismatches. This does not invalidate the fix, but it is an aspect that future reviews should inspect more closely.
I added an assertion to check if the time axes are strictly monotonous (which should always be the case anyways). Otherwise the np.interp function I use for resampling could fail silently (it just spits out wrong results). Other than that the changes don't make any assumptions about the shape of the data.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fork started with me not being able to run sensitivity analyses inside my OMEdit client because of a lookup issue inside the
varAnalysisForPerturbedParamfunction. In my case the float0.1000000001was compared against0.1resulting in a key error. I fixed this by looking up the nearest datapoint to the query. There were similar issues with therootMeanSquareForVarfunction. Both of these are insideindiv_sens.pyand are the only change that can be noticed from the user side.Every other change just adds typehints that make working with the code a lot more managable. I verified types by purposfully raising exceptions, then printing the type inside the error message. While doing this I noticed a lot of dead code, meaning classes that never get called during the analysis (from inside OMEdit), which are bloating up the module quite a bit.
I work with Python professionally and am eager to contribute more to this tool, as I see a lot of value in it. I'd also love to add more functionality in the future that I personally use in the field (i.e. an FFT Tool, Campbell plots, etc.).
(I couldn't get the tests to actually run, but I tested the 2 changes with live models inside OMEdit)
Related Issues
I couldn't find an issue relating to the key error crash yet