Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
surrogate model #77
base: main
Are you sure you want to change the base?
surrogate model #77
Changes from 1 commit
edc92c3
78b0dc8
b0c0653
6598753
c88e33e
650f621
5258ed4
e899df0
3ea64be
72eb782
68982cc
6226ca0
24321af
fed4207
6a7dd74
1210863
b6c5816
3960eef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is LatinHypercube here? could you document that somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the
harlow
implementation of LHS. I will try to improve the documentation and fix the docstrings asap.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very fond of having external imports in the same script where the base class is defined. I would personally keep the base class separated from the derived ones, creating one new script for each group of samplers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done for convenience initially, and it would be trivial to separate the specific implementations from the base class if necessary later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the sampler should have to treat the priors itself, probably better in the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the challenge with the implementation is that we strictly separated between the problem definition and compute functions (such as evaluating the prior with e.g. a scipy pdf evaluation, or a pytorch pdf evaluation), i.e. the problem definition cannot perform a sampling from the prior a priori. Thus, we would somehow have to create a solver just for sampling the prior using e.g. LHS. Not sure what exactly would be the recommended option, I see two options
In both situations, the intervals for the variables have to be transferred to the surrogate sampling either by having a check_bounds in the inference problem, or by "copying" or extracting these information from inference problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally a harlow sampler should be usable with a non-harlow surrogate, but it is not the case. Probably more a "harlow" thing than a "probeye" one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Harlow offers an abstract surrogate class which can be used to define new surrogates that can be used with the harlow samplers. Since most samplers need access to the surrogate (for fitting and making predictions iteratively), I don't think its feasible to make them work with any surrogate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why exactly should that not work with other surrogates? But I somehow agree that the sampler and the surrogate are connected and maybe we should actually follow a suggestions from Daniel to put that into a same base class.