Default values for Ensemble.from_sample

[PaulJonasJost]

fixes #727.

[codecov-commenter]

Codecov Report

Merging #730 (deaf478) into develop (9d84d3e) will decrease coverage by 0.41%.
The diff coverage is 54.54%.

@@             Coverage Diff             @@
##           develop     #730      +/-   ##
===========================================
- Coverage    89.74%   89.33%   -0.42%     
===========================================
  Files           99       99              
  Lines         6720     6730      +10     
===========================================
- Hits          6031     6012      -19     
- Misses         689      718      +29     

Impacted Files	Coverage Δ
pypesto/ensemble/utils.py	62.62% <ø> (-9.10%)	⬇️
pypesto/ensemble/ensemble.py	77.10% <54.54%> (-0.95%)	⬇️
pypesto/objective/amici_util.py	75.43% <0.00%> (-8.78%)	⬇️
pypesto/predict/amici_predictor.py	91.75% <0.00%> (-6.19%)	⬇️
pypesto/objective/amici_calculator.py	91.66% <0.00%> (-1.67%)	⬇️
pypesto/sample/geweke_test.py	94.36% <0.00%> (+2.81%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d84d3e...deaf478. Read the comment docs.

[jvanhoefer]

wouldn't we need some checks, if x_names, lower_bound, ... are in **kwargs?

[PaulJonasJost]

wouldn't we need some checks, if x_names, lower_bound, ... are in **kwargs?

Yes definitely currently on it. 👍

[dilpath]

The docstring for lower_bound in EnsemblePrediction looks incorrect (also seems completely different to upper_bound). Is lower_bound used with different meaning in different places or is this just a typo?

pyPESTO/pypesto/ensemble/ensemble.py

Lines 59 to 65 in e1d5640

	lower_bound:
	Array of potential lower bounds for the predictions, should have
	the same shape as the output of the predictions, i.e., a list of
	numpy array (one list entry per condition), with the arrays having
	the shape of n_timepoints x n_outputs for each condition.
	upper_bound:
	array of potential upper bounds for the parameters

[yannikschaelte]

it's just shortened, right? arguably too shortened, should at least say that the similar to lower bound.

[yannikschaelte]

ok thx for pointing out it's predictions vs parameters. agreed, while conceptually both may make sense, from usage it should probably be parameters.

[jvanhoefer]

Cool, looks good!

[PaulJonasJost]

Given the return statement of Ensemble.predict(), I agree, it should be parameters if anything, but does it even make sense? As far as I can see, the EnsemblePredictor does not contain any info about the parameters anymore and is just meant as simulation result. But in that case lower bounds for parameters do not make sense? Should I remove it entirely?

[dilpath]

Given the return statement of Ensemble.predict(), I agree, it should be parameters if anything, but does it even make sense? As far as I can see, the EnsemblePredictor does not contain any info about the parameters anymore and is just meant as simulation result. But in that case lower bounds for parameters do not make sense? Should I remove it entirely?

Seems like the intention is for bounds to be on parameters in Ensemble (see usage in Ensemble.check_identifiability:

pyPESTO/pypesto/ensemble/ensemble.py

Line 699 in f37f050

def check_identifiability(self) -> pd.DataFrame:

), and on predictions in EnsemblePrediction (currently no usage...). Hence it doesn't make sense to pass bounds from Ensemble to EnsemblePrediction, as done in Ensemble.predict:

pyPESTO/pypesto/ensemble/ensemble.py

Lines 666 to 667 in f37f050

	lower_bound=self.lower_bound,
	upper_bound=self.upper_bound

As @yannikschaelte said, both make sense, but only bounds on parameters are currently used. Maybe add a NotImplementedError to EnsemblePrediction.__init__ if bounds are passed? The names lower_bound and upper_bound could also be renamed in EnsemblePrediction to avoid confusion with the meaning of these names elsewhere in pyPESTO (and in Ensemble).