Subset arrays #411
Subset arrays #411
Conversation
…make the squeeze function and link it to the front end
There was a problem hiding this comment.
Thank you for the PR! The core of these changes already looks good, but there are some things I would like to see changed. Please also change the base branch of the PR to dev. See here on how to do that. EDIT: I could change it myself 🙂
| @@ -39,3 +39,6 @@ docs/ | |||
|
|
|||
| # MacOS | |||
| .DS_Store | |||
|
|
|||
| # Rproj | |||
| .Rproj.user | |||
There was a problem hiding this comment.
I am unfamiliar with R. What is this directory used for, and should all other users have it ignored too? Otherwise, please put this in your local .git/info/exclude instead.
There was a problem hiding this comment.
according to @stefanradev93, this should be .Rproj
tests/test_links/test_links.py
Outdated
| ) | ||
| assert not np.all( | ||
| np.diff(output, axis=i) > 0 | ||
| ), f"is ordered along axis which is not meant to be ordered: {i}." |
There was a problem hiding this comment.
I'm not sure why this is being reordered now, are you running ruff version 0.11.2?
There was a problem hiding this comment.
apparently I was running ruff 0.8.1 but I will update it
| def __init__(self): | ||
| super().__init__() | ||
|
|
||
| def forward(self, data: np.ndarray, sample_size: int, axis=-1): |
There was a problem hiding this comment.
IMO, the sample_size should be part of the constructor for this transform. It seems like a bit of a hassle to have to pass this argument in the forward call of the Adapter, unless you have a specific use case in mind?
There was a problem hiding this comment.
I would also allow it to be a float in [0, 1] specifying a proportion of the sample to subsample.
There was a problem hiding this comment.
I second this, but it raises the concern of whether we should floor or ceil the resulting value. I am thinking ceiling should be better.
There was a problem hiding this comment.
what case do you imagine for use of ceiling over floor? For most applications I assume users would want a slightly smaller rather than larger sample size.
LarsKue
added
feature
…r than internal shorthand
…ike the other transforms
There was a problem hiding this comment.
I'm honestly not sure that this should be a map transform as written, one consequence of the way its written now is that all keys specified by this transform will have random subsamples of the same size from the same axis. I think that all datasets that a user wants to subsample should be specified individually, so that axis and sample size are specified individually. If this is the case, would it not be better to force the map transform to reject a sequence of keys but rather only take one key?
… than only integer input
|
You also asked me to rename subsample_array to random_subsample my question is should all associated files also be renamed? |
github-project-automation
bot
moved this from In Progress
to Done
in bayesflow development
|
This was accidentally closed. We will investigate how to restore the branch and reopen PRs. |
|
Yes, please rename all associated files so the structure of
In this case, I think we would want to have a regular |
…sample and random_subsample respectively
bayesflow/adapters/adapter.py
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
These empty lines would be removed by the formatter, which should automatically run on pre-commit.
There was a problem hiding this comment.
i made a new environment based on the contribution.md
and now the linter runs automatically. on my end the most recent commit it passed all checks so im not sure why it's failing on the pull request
Codecov ReportAttention: Patch coverage is
|
|
I have no idea why the linter failed, i have it in the precommit hook and i merged dev into this branch. Im not really sure why it doesnt pass |
Addresses Issue #278
Implemented Take and subsample methods, however as discussed with Lars the requested squeeze functionality is essentially just the inverse of expand dims