Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi-subspace stepwise moves #65

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

dilpath
Copy link
Member

@dilpath dilpath commented Nov 15, 2023

This fixes a major bug in the stepwise logic.

Consider the model space table

model_subspace_id petab_yaml k1 k2
M1 full_petab_problem.yaml 1;estimate 1;estimate
M2 full_petab_problem.yaml 1;estimate 1

Let the current model in a backward search be in M1, with k1=1, k2=estimate.

Valid backward moves are k1=1 and k2=1 in M2, and k1=1 and k2=1 in M2.

M2 will never return k1=1 and k2=1. The current logic first checks the largest possible model in the space, then all combinations involving at least one "optionally fixed" parameter. k1 is not "optionally fixed" -- it must be fixed based on the current model. Hence, no model is returned from M2.

This PR changes the logic so that the first check is now "largest possible model in the space, but fix the parameters that were fixed in the other model subspace", and the corresponding fix for the forward moves.

An alternative fix would be to remove these lines from the backward method:

# The case of a "minimal" model in the subspace being a valid candidate
# in this case should have been handled above already with
# `new_must_estimate_all`
if not parameter_set:
continue

@dilpath dilpath merged commit 7c71053 into develop Nov 20, 2023
2 checks passed
@dilpath dilpath deleted the fix_multi_subspace_stepwise_moves branch November 20, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant