Open Daniel-Lin-S opened 2 months ago
Dear Thom, Thank you for your feedback on the ongoing projects. I have addressed all the points you mentioned. Regarding the notebook of problems, I believe Otto has already prepared one. Please let me know if there is anything else that needs attention or if further modifications are required. Best regards, Daniel
From: thomfoster @.> Date: Tuesday, 27 August 2024 at 14:14 To: thomfoster/treeQuadrature @.> Cc: Daniel Lin @.>, Author @.> Subject: Re: [thomfoster/treeQuadrature] Improved styles, made the package user-friendly, and added RbfIntegrator (PR #27)
@thomfoster approved this pull request.
This is great work Daniel. Happy to merge this into main.
Moving forward, we need to:
In treeQuadrature/container.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722186623:
- assert y.shape[1] == 1
def init(self, X: np.ndarray, y: np.ndarray, mins=None, maxs=None):
"""
Attributes
X : numpy.ndarray of shape (N, D)
each row is a sample
y : numpy.ndarray of shape (N, 1) or (N,)
the function value at each sample
mins, maxs : int or float or list or numpy.ndarray of shape (D,), optional
the low and high boundaries of the hyper-rectangle
could be +- np.inf
"""
if np.any(mins == np.inf):
raise ValueError(f'mins cannot have np.inf, got {mins}')
Why add this? What if I want to specify a container that is bounded only on 1 side for example?
In treeQuadrature/container.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722187311:
self.D = X.shape[1]
self.mins = np.array(mins) if mins is not None else np.array(
[-np.inf] * self.D)
self.maxs = np.array(maxs) if maxs is not None else np.array(
[np.inf] * self.D)
self.volume = np.product(self.maxs - self.mins)
self.mins = self._handle_min_max_bounds(mins, -np.inf)
self.maxs = self._handle_min_max_bounds(maxs, np.inf)
if self.mins.shape[0] != self.D:
Move shape checks inside _handle_min_max_bounds
In treeQuadrature/container.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722188757:
def add(self, new_X, new_y):
assert new_X.ndim == 2
assert new_y.ndim == 2
assert new_X.shape[0] == new_y.shape[0]
assert new_X.shape[1] == self.D
assert new_y.shape[1] == 1
assert np.all(new_X >= self.mins), new_X[new_X < self.mins]
assert np.all(new_X <= self.maxs), new_X[new_X > self.maxs]
new_X, new_y = self._handle_X_y(new_X, new_y)
if not np.all(new_X >= self.mins):
raise ValueError(f"Some values in new_X are below the minimum bounds: {new_X[new_X < self.mins]}")
Why do this when we've already thrown a warning?
In treeQuadrature/container.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722189899:
@@ -92,14 +216,43 @@ def X(self):
def y(self):
return self._y.contents
def rvs(self, n):
rs = np.random.uniform(size=(n, self.D))
ranges = self.maxs - self.mins
rs = self.mins + ranges * rs
def rvs(self, n: int):
nice!
On tests/test_container.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722193903:
Nice job adding these tests
On treeQuadrature/containerIntegration/containerIntegral.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722199765:
I don't love how we've done this - I'd quite like to refactor this such that ContainerIntegrals aren't classes, they are just functions that operate on Containers and return an integral number + results dict. What do you think?
In treeQuadrature/containerIntegration/constantIntegral.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722201903:
- '''estimate integral by the function value at mid-point of container'''
def containerIntegral(self, container: Container, f: callable):
mid_x = container.midpoint
mid_y = f(mid_x)[0, 0]
integral_estimate = mid_y * container.volume
return {'integral' : integral_estimate}
+class MedianIntegral(ContainerIntegral):
'''estimate integral by the median of samples in the container. '''
def init(self):
self.name = 'MedianIntegral'
def containerIntegral(self, container: Container, f: callable,
return_std: bool=False):
return_std breaks that containerIntegral's should always return a dict with a key of 'integral' that is a single float value. Should instead return {'integral': float, 'std': float} for the case where n==0
On treeQuadrature/containerIntegration/monteCarloIntegral.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722215573:
nice work
On treeQuadrature/gaussianProcess.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722216945:
This should be deleted right? in place of /gaussianProcess/gaussianProcess.py
In treeQuadrature/exampleProblems.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722218017:
-class Problem:
def init(self, D):
self.D = None
self.d = None
self.low = None
self.high = None
self.p = None
Attributes
D : int
dimension of the problem
lows, highs : np.ndarray
the lower and upper bound of integration domain
assumed to be the same for each dimension
this is a crazy thing to restrict to but I guess we're doing it ahaha
On treeQuadrature/exampleDistributions.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722227163:
I think this is confusing and maybe obselete now - do you agree? we can just merge this with exampleProblems and clean up?
In treeQuadrature/splits/uniformSplit.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722231604:
- n_divisions = kwargs.get('n_divisions', self.n_divisions)
+
if n_divisions <= 0:
raise ValueError("n_divisions must be a positive integer")
mins = container.mins
maxs = container.maxs
split_points = [np.linspace(mins[i], maxs[i], n_divisions + 1) for i in range(container.D)]
subcontainers = []
If this is slow and we want to speed it up I think we can remove the for loops and do as straight numpy
On treeQuadrature/samplers/sobolSampler.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722233705:
cool! Hadn't heard of this before
On treeQuadrature/samplers/mcmcSampler.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722237004:
Nice! Wild that you implemented this yourself - i would have definitely just used a library so I learnt something here!
On treeQuadrature/samplers/lowDiscrepancySampler.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722237802:
Replication of sobolSampler.py??
In treeQuadrature/integrators/vegasIntegrator.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722240924:
integ = vegas.Integrator([[-1.0, 1.0]] * problem.D)
f = ShapeAdapter(problem.pdf)
f = ShapeAdapter(problem.integrand)
Nice I like this change to using integrand
In treeQuadrature/integrators/limitedSampleIntegrator.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722292511:
@@ -26,72 +29,159 @@ def save_weights_image(q):
plt.close()
-class LimitedSampleIntegrator:
-
+class LimitedSampleIntegrator(TreeIntegrator):
Should this inherit from QueueIntegrator?
On treeQuadrature/integrators/limitedSampleGpIntegrator.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722293376:
Isn't most of this code already written in limited sample integrator?
On treeQuadrature/integrators/integrator.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722294465:
A nice addition here would be typing the return value of the integrators, ie returning a TypedDict or dataclass that we specify must contain the 'estimate' key
On treeQuadrature/integrators/gpTreeIntegrator.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722294959:
As with the limited sample integrator, let's clean this up and make use of code sharing
On treeQuadrature/gaussianProcess/visualisation.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1722296531:
nice
On treeQuadrature/container.pyhttps://github.com/thomfoster/treeQuadrature/pull/27#discussion_r1732825925:
I think we can just do numpy.stack :)
— Reply to this email directly, view it on GitHubhttps://github.com/thomfoster/treeQuadrature/pull/27#pullrequestreview-2246193090, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ASDZYQFGLRYYZ5O3OAAJLKTZTR3UZAVCNFSM6AAAAABK2MNULCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBWGE4TGMBZGA. You are receiving this because you authored the thread.Message ID: @.***>
container.py
added docstrings and comments added mins and maxs handling to allow arbitrary boundaries in each dimension added filtering of points outside the container using
filter_points
method modifiedrvs
to support infinite boundaries. changed Assertions to ValueErrors with clear error messagescontainerIntegration.py
added docstrings to existing methods added
rbfIntegral
added an abstract base classContainerIntegral
and modified existing methods to a classcontainerIntegration
, similar tosplits
andintegrators
added
gaussianProcess.py
with methods necessary to fit GP to a container
visualisation.py
added
plotContainers
that automatically plots all containers 1-dimensional problems are also also supported by_plotContainers1D
. addedplotIntegrand
to visualise the integrand addedplotGP
which plots the posterior mean in 1-D cases.edited
_plot2DContainers
method so that it supports plotting of any two dimensions for higher-dimensional problems.exampleDistributions.py
added docstrings
MixtureDistribution
changed to handle arbitrary weights created abstract class Distribution to allow User to define distribution fixedUniform.pdf
:low
andhigh
using_handle_low_high
method.added a test in
tests/test_distributions.py
to ensure Uniform.pdf returns correct values and Uniform.rvs samples are within boundsexampleProblems.py
added docstrings Added checks to
Problem.pdf
on the shape of X changed originalProblem
toBayesProblem
created a classProblem
for arbitrary problems with only anintegrand
addedGaussian
that represents integrating the pdf of any Multivariate Gaussian distribution on given boundaries addedPyramidProblem
that does not havervs
method to test samplers.added
sampler
,uniformSampler.py
,importanceSampler.py
,mcmcSampler.py
to accommodate for arbitrary problems without method
rvs
modifiedTreeIntegratror
so that whenproblem
does not havervs
,self.sampler
will be used. (defaults to UniformSampler) the corresponding tests can be found intests/test_samplers.py
utils.py
added docstring added exception handling to
scale
(prevent scaling constant array)added
integrators/integrator.py
an abstract base class
Integrator
for all other integrators test_integrators has been modified to accomodate the changesadded
integrators/treeIntegrator.py
an abstract base class inheriting
Integrator
, represents a tree-based integratorconstrct_tree
method that constructs a list of containers from a root container__call__
method is the same for all trees: aggregate the results in the trees.modified
__call__
to allow returning standard deviation estimation when containerIntegration method has return_std option. modified__call__
to return a dictionary, which is easier to use.modified the tests correspondingly
added
integrators/bayesMCIntegrator.py
performs Bayes Monte Carlo
and added to
tests/test_integrators.py
added
gpTreeIntegrator.py
a more powerful GP fitting scheme with parameter passing between containers.
and added to
tests/test_integrators.py
integrators/smcIntegrator.py
,integrators/vegasIntegrator.py
,integrators/limitedSampleIntegrator.py
,integrators/queueIntegrator.py
,integrators/simpleIntegrator.py
added docstrings with examples for users modified to inherit from
Integrator
added better type handling (avoid Users giving wrong types as arguments)LimitedSampleIntegrator
,QueueIntegrator
andSimpleIntegrator
now all inherit fromTreeIntegrator
addedverbose
option toTreeIntegrator
for easier debugging and tracking.added
splits/split.py
an abstract base class
Split
for all split methods for containersplits/uniformSplit.py
implemented
uniformSplit
splits/kdSplit.py
,splits/minSseSplit.py
added docstrings modified to inherit from
Split
added a check for X with less than 2 unique values. (cannot split anymore) inKdSplit
example_multivariate_normal.ipynb
import containerIntegrals from the package instead so that Users know how to use this package
tests/test_integrators.py
added test_io for all integrators added UniformSplit and RbfIntegral to the tests
added
compare_integrators.py
conveniently tests a set of integrators and problems, and writes the results into a csv file.