ood startpoints #732

yannikschaelte · 2021-10-03T10:42:01Z

Make startpoint methods object-oriented for clear API
Filling also resampling in there and getting rid of assign_startpoints and optimize options would be preferable, but didn't want to break API unless fine for all.

fixes #709

codecov-commenter · 2021-10-03T10:52:50Z

Codecov Report

Merging #732 (56d37b5) into develop (160c2a8) will decrease coverage by 54.00%.
The diff coverage is 27.67%.

@@             Coverage Diff              @@
##           develop     #732       +/-   ##
============================================
- Coverage    88.16%   34.16%   -54.01%     
============================================
  Files           79       99       +20     
  Lines         5257     6659     +1402     
============================================
- Hits          4635     2275     -2360     
- Misses         622     4384     +3762

Impacted Files	Coverage Δ
pypesto/engine/mpi_pool.py	`0.00% <0.00%> (ø)`
pypesto/ensemble/covariance_analysis.py	`18.36% <0.00%> (-0.39%)`	⬇️
pypesto/objective/aesara.py	`0.00% <0.00%> (ø)`
pypesto/objective/amici_calculator.py	`25.00% <0.00%> (-66.67%)`	⬇️
pypesto/optimize/__init__.py	`100.00% <ø> (ø)`
pypesto/petab/__init__.py	`0.00% <0.00%> (ø)`
pypesto/petab/importer.py	`0.00% <0.00%> (-85.90%)`	⬇️
pypesto/petab/pysb_importer.py	`0.00% <0.00%> (-100.00%)`	⬇️
pypesto/profile/profile.py	`32.43% <0.00%> (-59.46%)`	⬇️
pypesto/profile/util.py	`23.40% <ø> (-72.35%)`	⬇️
... and 125 more

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 7f696e3...56d37b5. Read the comment docs.

dweindl · 2021-10-04T08:26:34Z

Filling also resampling in there and getting rid of assign_startpoints and optimize options would be preferable, but didn't want to break API unless fine for all.

No objections.

FFroehlich · 2021-10-04T10:03:02Z

pypesto/startpoint/__init__.py

-
+Methods for selecting points that can be used as startpoints
+for multi-start optimization.
+Startpoint methods can be implemented as derived from


Suggested change

Startpoint methods can be implemented as derived from

Startpoint methods can be implemented by deriving from

FFroehlich · 2021-10-04T10:10:55Z

pypesto/startpoint/assign.py

+                lb=lb,
+                ub=ub,
+                objective=objective,
+                x_guesses=x_guesses


I believe this will lead to issues when non-empty x_guesses is passed since n_starts is always set to one and the startpoint index is not provided. I think startpoint_method should really only use x_guesses as reference and any selection of startpoint vs guess should be carried out in this function and assign_startpoints. Along these lines this function should probably issue a warning when resampling a user-provided guess.

not sure if I understand. the startpoint_method is simply a uniform or lhs function, so atm no startpoint_method actually uses x_guesses. They could use it to only suggest points away from those guesses, but that only makes a limited amount of sense, so atm if would be no problem to just remove x_guesses from the startpoint_method.__call__.

According to the docstring in StartpointMethod, the respective methods are supposed to account for x_guesses.

ah yes, that was a copy-pasta error. the StartpointMethod is not supposed to return any of these, but only conceptually use them to e.g. only sample remote points. xguesses was passed in the original formulation but never used. to avoid issues here I think I'll just remove the argument.

FFroehlich · 2021-10-04T10:11:14Z

pypesto/startpoint/base.py

+        x_guesses:
+            Externally provided guesses, shape (n_guess, n_par).
+            Maybe used as reference points to generate remote points (e.g.
+            maximizing some distance). If n_guesses >= n_starts, only the first


Suggested change

maximizing some distance). If n_guesses >= n_starts, only the first

maximizing some distance).

FFroehlich · 2021-10-04T10:11:28Z

pypesto/startpoint/base.py

+            Externally provided guesses, shape (n_guess, n_par).
+            Maybe used as reference points to generate remote points (e.g.
+            maximizing some distance). If n_guesses >= n_starts, only the first
+            n_starts guesses are returned.


Suggested change

n_starts guesses are returned.

FFroehlich · 2021-10-04T10:15:28Z

pypesto/optimize/optimize.py

+            "Problem.startpoint_method will be ignored. Startpoints will be "
+            "generated using the startpoint method given as an argument to "
+            "the minimize function.",
+        )
    elif problem.startpoint_method is not None:


add a switch if isinstance(startpoint_method, Callable) that instantiates a FunctionStartpoints and issues a deprecation warning to mantain backwards compatibility?

FFroehlich · 2021-10-04T10:16:37Z

pypesto/optimize/optimize.py

+    optimizer: Optimizer = None,
+    n_starts: int = 100,
+    ids: Iterable[str] = None,
+    startpoint_method: Union[StartpointMethod, bool] = None,


I have to say that I find it a bit counterintuitive to pass a class object to an argument that is called "..._method"

yeah ... was to keep it consistent with before. Effectively, it is a method with maybe some hyperparameters that are memorized via self reference ^^

jvanhoefer

Nice work!

jvanhoefer · 2021-10-04T09:52:28Z

pypesto/startpoint/base.py

+        startpoints:
+            Startpoints, shape (n_starts, n_par).
+        """
+


Suggested change

raise NotImplementedError()

not necessary as method is declared as abstract. further, I would argue that "NotImplementedError" is for when functionality is just still missing (e.g. due to developer's laziness), while abstractmethod is for stuff that is not actually meant to be implemented in this place.

jvanhoefer · 2021-10-04T09:54:46Z

pypesto/startpoint/base.py

+        """
+
+
+class FunctionStartpoints(StartpointMethod):


Should we put that in a separate function_startpoint.py file?

could, but as this is also very core functionality, I'd just keep it in here, to have not too many files

jvanhoefer · 2021-10-04T10:11:20Z

pypesto/startpoint/assign.py

+        Startpoints, shape (n_starts, n_par).
+    """
+    # check if startpoints needed
+    if startpoint_method is False:


Is in contrast with the type annotations...

jvanhoefer · 2021-10-04T10:12:34Z

pypesto/startpoint/assign.py

+
+def assign_startpoints(
+    n_starts: int,
+    startpoint_method: StartpointMethod,


Can apparently also be a bool.

true, added

…_startpoints

ood startpoints; fixes #709

63ee468

yannikschaelte requested review from FFroehlich and jvanhoefer October 3, 2021 10:42

yannikschaelte self-assigned this Oct 3, 2021

yannikschaelte added 2 commits October 3, 2021 12:42

fix typo

616aa28

add file docstrings

e61b5eb

Merge branch 'develop' into fix_startpoints

b4f7ff7

FFroehlich approved these changes Oct 4, 2021

View reviewed changes

jvanhoefer approved these changes Oct 4, 2021

View reviewed changes

yannikschaelte and others added 8 commits October 5, 2021 15:10

Merge branch 'develop' into fix_startpoints

749f50c

Merge branch 'develop' into fix_startpoints

7271c2a

reviewer comments

2072e94

Merge branch 'develop' into fix_startpoints

2e2824d

reviewer comments 2

becbff4

Merge branch 'fix_startpoints' of github.com:icb-dcm/pypesto into fix…

179930d

…_startpoints

fix args

29a55a7

fix sympy

56d37b5

yannikschaelte merged commit 18960c0 into develop Oct 12, 2021

yannikschaelte deleted the fix_startpoints branch October 12, 2021 13:42

yannikschaelte mentioned this pull request Oct 28, 2021

Release 0.2.8 #740

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ood startpoints #732

ood startpoints #732

yannikschaelte commented Oct 3, 2021

codecov-commenter commented Oct 3, 2021 •

edited

Loading

dweindl commented Oct 4, 2021

FFroehlich Oct 4, 2021

FFroehlich Oct 4, 2021

yannikschaelte Oct 7, 2021

FFroehlich Oct 7, 2021

yannikschaelte Oct 8, 2021

FFroehlich Oct 4, 2021

FFroehlich Oct 4, 2021

FFroehlich Oct 4, 2021

FFroehlich Oct 4, 2021

yannikschaelte Oct 9, 2021

jvanhoefer left a comment

jvanhoefer Oct 4, 2021

yannikschaelte Oct 9, 2021

jvanhoefer Oct 4, 2021

yannikschaelte Oct 9, 2021

jvanhoefer Oct 4, 2021

jvanhoefer Oct 4, 2021

yannikschaelte Oct 9, 2021

	Startpoint methods can be implemented as derived from
	Startpoint methods can be implemented by deriving from

	maximizing some distance). If n_guesses >= n_starts, only the first
	maximizing some distance).

ood startpoints #732

ood startpoints #732

Conversation

yannikschaelte commented Oct 3, 2021

codecov-commenter commented Oct 3, 2021 • edited Loading

Codecov Report

dweindl commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvanhoefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 3, 2021 •

edited

Loading