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

Pending experiments with CustomDiscreteParameter value not in search space gives cryptic error #453

Open
mhrmsn opened this issue Dec 18, 2024 · 2 comments · May be fixed by #456
Open

Pending experiments with CustomDiscreteParameter value not in search space gives cryptic error #453

mhrmsn opened this issue Dec 18, 2024 · 2 comments · May be fixed by #456
Labels
enhancement Expand / change existing functionality

Comments

@mhrmsn
Copy link

mhrmsn commented Dec 18, 2024

Given a Recommender with a SearchSpace.discrete with a CustomDiscreteParameter using a user-specified custom embedding, it is possible to use Recommender.recommend(..., pending_experiments=pending), where pending contains values for that custom parameter that are not in the search space (and have no embedding).

This will then fail with a cryptic error message like this (probably depends on acquisition function, etc.):

raise NanError(f"cholesky_cpu: {isnan.sum().item()} of {A.numel()} elements of the {A.shape} tensor are NaN.")
linear_operator.utils.errors.NanError: cholesky_cpu: 17040384 of 17040384 elements of the torch.Size([64, 516, 516]) tensor are NaN.

I think it might be nice to catch this and raise a more descriptive error instead.

In addition: Not sure if this also happens for measurements with the same issue, and more generally, if I wanted to add measurements with CustomDiscreteParameter values outside of the search space, how would you go about this? Add them and then mark the corresponding values as dont_recommend?

@AdrianSosic
Copy link
Collaborator

Hi @mhrmsn, thanks for the info. The problem is largely related to a few design issues that are already known and for which a fix is already on the way. In particular, handling values of "categorical parameters" (of any form, i.e. also substance and custom) that are undefined is a general pattern that needs to be addressed. Our current plan is to rigorously apply the active_values concept that already exists for TaskParameters also to other types of categorical parameters. In addition, there is a currently a major refactoring in progress that substantially improves the control of recommendable points, i.e. the entire dont_recommend thing will be replaced with a proper user-facing interface that lets people toggle points in the candidate set, i.e. temporarily exclude them from recommendation. (see #412 and #423)

Together, these two pieces will mostly address the problems you've described above. But a fundamental limitation will remain: it will still be impossible to add measurements with categorical values that are undefined. I think this can't be solved from the BayBE side since it really results in an undefined situation. In this case, raising a clear error message will be the only option.

But let me know if you have better ideas / suggestions. Also, could you perhaps add a minimal example here for tracking, so that I can use it as a test case and mark the issue resolved once all problems have been addressed?

@AdrianSosic AdrianSosic added the enhancement Expand / change existing functionality label Dec 18, 2024
@Scienfitz
Copy link
Collaborator

Scienfitz commented Dec 18, 2024

Seems this is a user-caused problem (accidentally trying to provide pending data for undefined labels), but the code should better handle it in full analogy to normal measurements.

  • validation has been mostly forgotten for pending_experiments and included values, it essentially needs to the same simple validation as data in add_measurements
  • I think the corresponding fix is probably small, no redesign of any sort needed. I estimate 20-40 lines, largely copied from add_measurements and fuzzy_row_match. Maybe it could be consolidated into a utility too
  • yes, the future change to values and active values will affect this too, but is in no way blocking to fix this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Expand / change existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants