Draft: merge optional andie backend#31
Conversation
…int issue; added client stopping criterion when the last grid point is sampled
…emperature range 100-500K; updated temperature grid and mock intensity-temperature profile in andie_mock.py for temperature range 100-500K
Signed-off-by: Lance-Drane <Lance-Drane@users.noreply.github.com>
…nto the dependency; removed pdm.lock
… script andie_general_mock.py, the caveat is that the sampling method is still hardcoded since it depends on the specific problem
There was a problem hiding this comment.
should be removed?
There was a problem hiding this comment.
yeah *.out should also be added to .gitignore
There was a problem hiding this comment.
Is there a reason to keep andie_backend and andie_backend_general? We could just have a default parameter dict for general?
There was a problem hiding this comment.
Once andie_backend_general is tested, we may remove the andie_backend option. I'm not sure if there is a need for the generalized version though.
There was a problem hiding this comment.
@Lance-Drane can we move these to optional dependencies right now?
There was a problem hiding this comment.
sure; I can either make a push directly to this branch which changes this, or I can make a separate branch and make a MR into this branch
| "pymongo>=4.12.1", # TODO - this is only needed for dial_service, dial_dataclass can use a simple fixture for ObjectID representation which allows it to skip this dependency | ||
| "sable @ git+https://code.ornl.gov/sable/sable.git", # TODO this references the internal repo, transition to public version | ||
| "bumps>=0.9.3", # TODO consider making an optional dependency group | ||
| "bump>=1.4.0", |
There was a problem hiding this comment.
Do we need this package? https://pypi.org/project/bump/
There was a problem hiding this comment.
It certainly looks like a typo for bumps to me. I'm not seeing any new code in this PR that uses bump.
There was a problem hiding this comment.
Right, it is a typo from an early pip freeze and should be removed.
| available_backends = [ | ||
| bkend for bkend in _POSSIBLE_BACKENDS if importlib.util.find_spec(bkend) is not None | ||
| bkend for bkend in _POSSIBLE_BACKENDS | ||
| ] |
There was a problem hiding this comment.
We can modify the old logic slightly from the old implementation; we should probably have a mapping of backend names to required imports. (It makes sense to call the backend andie but the old logic assumed that the name of the import package itself is andie.) I can add this change myself, either directly on this branch or in a MR targeting this branch; I would probably also include the optional dependency stuff in the same MR.
(A backend could require any number of non-stdlib packages being available, and a backend may not necessarily require any non-stdlib packages being available.)
There was a problem hiding this comment.
I don't think we need to call the backend andie -- essentially this is just an example of MCMC-based Bayesian inference of parameters for a fixed functional form.
I'm open to other names, but I'd consider calling this functional_form or something along those lines.
There was a problem hiding this comment.
I don't think we need to call the backend andie -- essentially this is just an example of MCMC-based Bayesian inference of parameters for a fixed functional form.
I'm open to other names, but I'd consider calling this functional_form or something along those lines.
functional_form is fine with me (I should not be in charge of naming things, so I am fine with whatever is decided!). I can go ahead and make a separate MR for this targeting develop, then this branch can rebase/merge into develop and add a single line to the mapping I'll generate.
| ), | ||
| ] | ||
| kernel: Literal['rbf', 'matern', 'linear'] | ||
| kernel: Literal['rbf', 'matern', 'linear', None] |
There was a problem hiding this comment.
Having None as the kernel is probably okay, as IIRC we are eventually going to change the API to not have the user provide this (?). However, we need to verify that nothing else breaks from doing this.
There was a problem hiding this comment.
If I'm interpreting this correctly, it's that we currently have kernel as a required parameter, but the andie approaches don't have a kernel, so None is most appropriate.
| 'upper_confidence_bound_nomad', | ||
| 'polymer_acl_sampler', | ||
| ] | ||
| None] |
There was a problem hiding this comment.
this on the other hand I don't like, I think that a user should always specify a strategy to use and this should never be empty or default.
Similarly to the kernel type, I think a long-term goal (not to be tackled in this MR) is to encapsulate the user from directly selecting a backend; however, we can always add more strategy values that we accept. For the moment, we can check to make sure that the strategy/backend combination is supported in the various core functions with simple control flow branching, probably after first checking that the strategy isn't random or hypercube (since these strategies don't even use any of the backends).
There was a problem hiding this comment.
My instinct is to agree with @Lance-Drane here -- I'd expect the user to always specify a strategy. The only case where I could see None being appropriate is if we just want to return the surrogate value without doing any active learning.
There was a problem hiding this comment.
The only case where I could see None being appropriate is if we just want to return the surrogate value without doing any active learning.
It's still probably better to give an explicit strategy name instead of relying on a user being able to parse what a "null strategy" means (on the INTERSECT side, "None" always translates to "null").
5 participants
Draft MR: Add ANDIE as an optional backend.