Update unsequa to fully support the latest version of SALib

[luseverin]

Changes proposed in this PR:

• Update SALib sensitivity and sampling methods from newest version (SALib 1.4.7) #828
• Allow for computation of relative and absolute delta impacts in CalcDeltaClimate

This PR fixes #828

PR Author Checklist

• Read the Contribution Guide
• Correct target branch selected (if unsure, select develop)
• Descriptive pull request title added
• Source branch up-to-date with target branch
• Documentation updated
• Tests updated
• Tests passing
• No new linter issues
• Changelog updated

PR Reviewer Checklist

• Read the Contribution Guide
• CLIMADA Reviewer Checklist passed
• Tests passing
• No new linter issues

[luseverin]

A few more comments on this PR:

• Three new sensitivity methods required a specific treatment to be included into unsequa: ff, hdmr, and delta
• The output of the ff and hdmr methods from SALib required refactoring in order to fit unsequa's _sens_df output format. I had to remove a few variables from the output of hdmr as I could not find a way to refactor them to the _sens_df. These include notably all the details of the surrogate model used, included in the Em (Emulator) variable. Additionally, all output variables from hdmr are refactored to 2D arrays, even the first-order sensitivity indices.
• The delta method systematically failed with a matrix inversion error: LinAlgError: Singular matrix detected This may be due to the sample size (30000) being too small. If this is not the case, check Y values or raise an issue with the SALib team. I could not find why that was so I removed delta from the compatible sensitivity methods.
• I also have some doubts on the stability of the hdmr method as it seem to yield different results between identical model runs. I decreased the sensitivity of the AssertEqual tests in the unit test in consequence so that it should not be failing.

[chahank]

@emanuel-schmid : why would we indicate two different larger than versions (as for geopandas)?

[emanuel-schmid]

This is about dependency version constraint changes. They start from the given minimal version of the last release (left hand side) and end at the given minimal version of this release (right hand side).
However: the whole version change section is eventually done at release time by a script I wrote. Don't bother, it will be overwritten!

[chahank]

I think we should rather explicitly catch the case for dummy_ variables and only ignore these instead of all possible params.

[luseverin]

Do we still need this line of code? As we now raise a ValueError when the number of parameters is not a power of 2 and require the user to input their own dummy parameter beforehand. Then if the user inputs their own dummy parameter, the dummy parameters should be covered by the df_samples[param] = df_samples[param].apply(self.distr_dict[param].ppf), correct?

[chahank]

Why not use the attribute sensitivity_method? This would be more stable I think.

[luseverin]

Because it is not available from within _calc_sens_df, I think? But we can pass it as an argument of _calc_sens_df if you think it is worth it.

[chahank]

hmmm not sure. But maybe then just check for the last 2 strings ff. Because I vaguely remember that Salib had played around with changing the module structure, such that it would not be Salib.analyse.ff but something Salib.xxx.ff.

[luseverin]

Ok then I changed it to only check if the corresponding last characters of the method name string correspond to the target method name instead of the entire Salib.analyse.ff.

[luseverin]

Great, thanks!

I think there are a few small things to update, and maybe one general idea for the tests. Since the tests for the different method repeat themselves, it would be better to define a function and then call it in a loop for all methods instead of copying large parts of the code.

Ok so I included your suggested changes + I added a prototype of a generic testing function for testing the different sensitivity methods. I store all the parameters and expected tests results in a dict which I pass to the function that gets looped over. Maybe not the most efficient way to do it so please let me know if you have any suggestions for improvements.

[chahank]

Great job, we are almost there! What is missing:

• Address all the new linter issues (and any older ones that you solve are bonus points!)
• The method for the test is in the right direction. I would maybe propose to simplify the whole thing a bit. More on that in the code directly.

[luseverin]

Ok so I tried to simplify the tests of the sensitivity methods as much as possible in the generic test functions, and removed the previous method-specific test functions. I wasn't sure about a few lines that only appeared in the test for the morris method though:

for name, attr in unc_data.__dict__.items():
            if 'sens_df' in name:
                np.testing.assert_array_equal(
                    attr.param.unique(),
                    np.array(['x_exp', 'x_paa', 'x_mdd'])
                    )
                np.testing.assert_array_equal(
                    attr.si.unique(),
                    np.array(['mu', 'mu_star', 'sigma', 'mu_star_conf'])
                    )
                if 'eai' in name:
                    self.assertEqual(
                        attr.size,
                        len(unc_data.param_labels)*4*(len(exp_unc.evaluate().gdf) + 3)
                        )
                elif 'at_event' in name:
                    self.assertEqual(
                        attr.size,
                        len(unc_data.param_labels) * 4 * (haz.size + 3)
                        )
                else:
                    self.assertEqual(len(attr),
                                     len(unc_data.param_labels) * 4
                                     )

I did not include these lines in the generic test function as it did not seem absolutely necessary to me, but if you think those should be included just let me know.

Additionally, I need to fix the few new linter issues that popped up but I think it shouldn't be an issue. Otherwise I think I adressed the rest of your comments.

[chahank]

Is this something that still needs to be addressed or is it a left-over?

[luseverin]

It's a left-over, I cleaned it up.

[chahank]

Any particular reason for renaming the logger? Why not just use LOGGER ?

[luseverin]

No, no reason, it was just blind copying of the code I saw on stackoverflow, I'll change that!

[luseverin]

Ok so I incorporated your last suggestions. If there is no further comments from your side I suggest we merge this PR?

[chahank]

I think it looks good!

Just FYI: you managed to remove 39 pylint warnings, and add 16. Maybe you can take a last look whether the 16 new ones are easy to avoid or not.

Else, this is ready to merge from my point of view.

[luseverin]

So I have checked and the warnings are mainly "too-many-locals", "too-complex", "invalid-name", etc. Most of those are coming from lines that I not directly touched or that got included to the commits because of trailing whitespaces that got removed by VScode. For instance, plot_rp_uncertainty is too complex, or they are too many local variables in Calc_delta_climate.uncertainty. It is not straightforward to me how these should be treated so I would rather leave them as is for the moment if that's fine..

[chahank]

Thanks for checking. For the moment these are fine I think.

Feel free to merge, and thanks for the work!