Skip to content
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

Made Adapters Sliceable #285

Merged
merged 9 commits into from
Jan 15, 2025
Merged

Made Adapters Sliceable #285

merged 9 commits into from
Jan 15, 2025

Conversation

eodole
Copy link
Collaborator

@eodole eodole commented Dec 17, 2024

I made the adapters sliceable but one concern I have is that when I ask for generic_adapter[0:2] this will return an entirely new adapter instance. Depending on the use case this one matter but could cause problems. I also made a test file but it's not tracked by github, please let me know if I should also upload that.

I also noticed that ElementwiseTransform wasn't being imported so added that as well.

I thought it might also be nice to add the indices of the transforms to the default adapter print, but didn't add that particular feature. I also thought it might be nice to add the feature of appending a new transform via slice assignment so if index == len(adapter.transforms) then simply use the add_transform method.
Resolves #274

…ed to define set item, might be nice to list indecies of transforms in the default print
… transform was not added to the list for things to imported in the adapter file so added that
@paul-buerkner
Copy link
Contributor

Thank you! I will take a look soon!

@paul-buerkner
Copy link
Contributor

paul-buerkner commented Dec 18, 2024

A few short questions:

  • Where do you anticipate issues with slicing creating new adapter instances? To me it sounds intuitive that we would create a new adapter instance.
  • A test file (if set up in the appropriate test folder etc.) would be great and should be added to this commit I think.
  • Index numbers in the printing would be great! How much effort would it be for you to add this?
  • I don't fully understand what you mean by "appending a new transform via slice assignment so if index == len(adapter.transforms) then simply use the add_transform method." Can you elaborate? Also perhaps use some example code how what you have in mind would look?

@eodole
Copy link
Collaborator Author

eodole commented Jan 1, 2025

Essentially what i meant with my last comment is that you can add a transform to the end of the adapter by slicing.
I have to find the code responsible currently for printing but I think the modification should be pretty easy

@paul-buerkner
Copy link
Contributor

Ah, I see. Could you show a code example? Let me know once you have added everything you wanted to add to this PR.

@LarsKue
Copy link
Contributor

LarsKue commented Jan 2, 2025

Appending via slicing is fine, imo. We should have checks in place and error if slices are assigned non-Adapter objects (thus making the adapter self-recursive, each slice is an Adapter itself).

That said, best practice should be to use add_transform (could just call this append as well). We could add an insert method to mimic list behavior, too.

Copy link
Contributor

@LarsKue LarsKue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, see individual comments for potential changes and discussion.

bayesflow/adapters/adapter.py Outdated Show resolved Hide resolved
bayesflow/adapters/adapter.py Outdated Show resolved Hide resolved
bayesflow/adapters/adapter.py Outdated Show resolved Hide resolved
bayesflow/adapters/adapter.py Outdated Show resolved Hide resolved
bayesflow/adapters/adapter.py Outdated Show resolved Hide resolved
bayesflow/adapters/adapter.py Outdated Show resolved Hide resolved
bayesflow/adapters/adapter.py Outdated Show resolved Hide resolved
@paul-buerkner
Copy link
Contributor

@eodole Did you address all the comments by lars already? If yes, can you resolve them? If not, can you make the necessary edits? Let me know if you have questions.

@LarsKue LarsKue self-requested a review January 15, 2025 13:34
Copy link
Contributor

@LarsKue LarsKue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the changes. I added and modified some parts so that we can fully treat the Adapter like a list now.

@LarsKue LarsKue merged commit 3e7cea5 into bayesflow-org:dev Jan 15, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants