Add reading Ensemble features from optimization result #640
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #640 +/- ##
===========================================
- Coverage 89.82% 87.35% -2.47%
===========================================
Files 94 94
Lines 6013 6081 +68
===========================================
- Hits 5401 5312 -89
- Misses 612 769 +157
Continue to review full report at Codecov.
|
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
…into feature_ensembles
| 'Either the cutoff value was too small or the ' | ||
| 'result.optimize_result object might be empty.') | ||
| elif len(x_vectors) < max_size: | ||
| logger.info(f'The ensemble contains {len(x_vectors)} parameter ' |
There was a problem hiding this comment.
Would make this a warning rather than an info, but that's a bit of a matter of taste...
There was a problem hiding this comment.
would leave that to @dilpath in a democratic vote then 👍
…into feature_ensembles
| 'Either the cutoff value was too small or the ' | ||
| 'result.optimize_result object might be empty.') | ||
| elif len(x_vectors) < max_size: | ||
| logger.info(f'The ensemble contains {len(x_vectors)} parameter ' |
pypesto/ensemble/ensemble.py
Outdated
| n_starts += 1 | ||
|
|
||
| if n_starts*max_per_start > max_size: | ||
| logger.info(f'The number of starts that can contribute an ' |
There was a problem hiding this comment.
not sure how this looks on screen, but consider also using line breaks after e.g. 80 chars
Co-authored-by: Yannik Schälte <31767307+yannikschaelte@users.noreply.github.com>
Co-authored-by: Yannik Schälte <31767307+yannikschaelte@users.noreply.github.com>
…iscussion points from last meeting.
|
maybe before merging a more explanatory PR title would be nice (for future references, e.g. when preparing a new release) |
pypesto/ensemble/ensemble.py
Outdated
| indices = np.round(np.linspace(0, len(candidates)-1, n_per_start)) | ||
| return candidates[indices.astype(int)] |
There was a problem hiding this comment.
Maybe convert to set? Otherwise is it possible that e.g. len(candidates)-1==1 and n_per_start==6 so indices==[0, 0, 0, 1, 1, 1] and so the returned vector indices are duplicates?
There was a problem hiding this comment.
no n_per_start will give you a number that will definitely be lower than len(candidates) as per code so this should not be a problem. I do like lists more but that is a personal preference :D
There was a problem hiding this comment.
Ah, if it's not intended for a user to call this method directly, then the name can be prefixed with a _ e.g. _indices_per_start
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
PaulJonasJost
changed the title
| @@ -594,3 +749,97 @@ def check_identifiability(self) -> pd.DataFrame: | |||
| parameter_identifiability['parameterId'] | |||
|
|
|||
| return parameter_identifiability | |||
|
|
|||
|
|
|||
| def entries_per_start(fval_traces: List['np.ndarray'], | |||
There was a problem hiding this comment.
Wouldn't "Sequence" do the trick here? Why do we sometimes give the typehint for Sequence and sometimes for List? There are no list-specific features used here... Actually, we only need "Iterable", right?
There was a problem hiding this comment.
it would. note subtle differences, e.g. numpy.ndarray is an Iterable but no Sequence
| n_per_start = [min(n, max_per_start-decr) | ||
| for n in n_per_start] | ||
| decr += 1 |
There was a problem hiding this comment.
With a little hack on this, we could do something which is fully satisfying... However, I think that's fine from my side for the current version
There was a problem hiding this comment.
what would you have in mind? :)
There was a problem hiding this comment.
Something that exactly matches max_size whenever possible?
There was a problem hiding this comment.
no I meant what would your little hack be 🙂
New features: