-
Notifications
You must be signed in to change notification settings - Fork 58
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
rfc: replace synthesizer function with class #89
Comments
@csala Any thoughts on this proposal? |
Hi @sbrugman I think that for now the exact change that you are proposing is not within the current SDGym roadmap, but some variation is:
To add some more context to it, the reason for which the required input is a On the other hand, it is true that this support for class-based synthesizer is not really documented, so adding it to the docs would be interesting!
This is another story, and it could actually be interesting too! An option that would be acceptable without sacrificing the functional input, would be to modify the code to capture such metrics only when the provided synthesizer is a |
Hi Carles, thanks for getting back at this. The clarification on why you would not like to impose the |
Hello, I'm jumping in here a few years after this initial conversation. Since 2021, we have made significant updates to the usage/API of SDGym as well as its functionality. And I believe that some key features that were discussed here have now been incorporated into the library.
So I'm going to mark this issue as (more-or-less) resolved. Apologies if I've overlooked any of the finer points in the discussion. If there is more to add, I'd recommend filing a new issue and we can start a new discussion based on the vision and capabilities of the newest SDGym library. Thanks! |
Description
SDGym's synthesizers all inherit from the Baseline class (or BaseSynthesizer class in previous versions). Users can provide custom synthesizer functions. The convenience inheritance is demonstrated throughout SDGym's code base and has all sort of other benefits. My suggestion would be to make the following changes:
fit
andsample
methodThese changes provide consistency between SDGym's native and user-provided synthesizers and clear distinction between fit and sample logic, at nearly no cost:
will become
More interestingly, this structure allows for capturing valuable metrics that are currently out of reach related to fit/sampling time and complexity (time measurements or maybe even this package). SDGym would this way be able to benchmark this aspect of a synthesizer as well, which can be an important decision criterion for which synthesizer is best for a given use case: if the user expects to sample large quantities of data then a longer fitting time would be acceptable at a lower sampling complexity.
The code that needs to be changed for this is minimal, however I wanted to make sure you see value in this point before drafting a PR.
The text was updated successfully, but these errors were encountered: