-
Notifications
You must be signed in to change notification settings - Fork 4
add mvbart to _interface.py #63
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
base: main
Are you sure you want to change the base?
add mvbart to _interface.py #63
Conversation
|
Do everything in Bart, I guess it should be possible by adding a few helper methods. P.S.: If you find it easier, make a completely separate version work like you are doing right now, then we can merge them together later. |
|
Btw I'm about to merge #62 which, amongst other things, adds multivariate support throughout |
I've move everything to Bart. btw just curious why in your mcmcloop. evaluate_trace function the outputed y is of dimension n*k? i thought you would prefer k first. |
|
Ach right, I flipped them, I'm going to fix that. |
Gattocrucco
left a comment
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 quickly read your code and just noted down the easy things I could notice immediately, I still haven't checked the thing in detail.
| offset: FloatLike | None = None, | ||
| lamda: FloatLike | None = None, # to change? | ||
| tau_num: FloatLike | None = None, # to change? | ||
| offset: FloatLike | None = None, # to change? |
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.
For offset: also support shape (k,)
| lamda: FloatLike | None = None, # to change? | ||
| tau_num: FloatLike | None = None, # to change? |
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.
For lambda and tau_num: try to work out a good multivariate generalization of this thing.
I guess tau_num can be a scalar, with leaf_prior_cov_inv just being 1/sigma_mu^2 on the diagonal, that part is fine as is.
For error_cov_scale you do multivariate linear regression and get the estimate of the error cov matrix, but then what to do with the chisquared quantile thing used for univariate? I guess there should be something with the maximum eigenvalue of the error covariance matrix, i.e., set the quantile on the weighted outcome average with the largest residual variance.
I haven't read in detail your code so maybe you've already done the above, but I need to go to bed now.
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.
For lambda and tau_num: try to work out a good multivariate generalization of this thing.
I guess tau_num can be a scalar, with leaf_prior_cov_inv just being 1/sigma_mu^2 on the diagonal, that part is fine as is.
For error_cov_scale you do multivariate linear regression and get the estimate of the error cov matrix, but then what to do with the chisquared quantile thing used for univariate? I guess there should be something with the maximum eigenvalue of the error covariance matrix, i.e., set the quantile on the weighted outcome average with the largest residual variance.
I haven't read in detail your code so maybe you've already done the above, but I need to go to bed now.
For tau_num: Currently, I allow it to be a vector (length
For error_cov_scale: I currently mimic the univariate logic for each dimension independently. I run the standard quantile calibration on each row of
However, I was also thinking about using the info from eigenvalues and make the scale matrix reveal some info about correlation of y's. Any preference or comments?
|
I merged #64 which created some conflicts with you, sorry! Most of the conflicts look easy to solve, the only tricky one is _predict(). The new version from main is simpler (it does not use the auxiliary _blahblah_chain methods), but you'll have to adapt it again. |
i changed it to |
|
Hi, I merged #72 which created a small conflict! Please drop |
I've got the multivariate logic running and passing the linter (for now I’m using # noqa to call a few internal helpers).
At the moment, MvBart reuses some private methods from Bart. I am thinking about either 1.factoring shared pieces into a public BaseBart (so Bart and MvBart inherit from it), or 2. extending Bart directly to support both univariate and multivariate cases. Which way do you prefer?
I still need to add some unit tests on convergence and sanity checks. will do it soon.