-
Notifications
You must be signed in to change notification settings - Fork 0
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
FIST sampling #45
base: develop
Are you sure you want to change the base?
FIST sampling #45
Conversation
I have extracted the functionalities for the separate samplers in separate files. I will add the documentation once a preliminary review finds that the implementation is okay. |
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.
@NGoetz You did an excellent job so far. I am amazed of how few code changes are needed (very good example of the OCP).
I left around lots of comments about the implementation, I did not yet review in detail the black-box and the tests.
Regarding unit tests, do you think it is necessary to duplicate all unit tests for both samplers?
As rule of thumb, the units to be tested are the specific functions. Then tests for the generic sampler functionality are more integration tests and I would say we can be less meticulous there. So the rationale is: "Let's test in a solid ways ingredients and have at least one meaningful test for their integration".
As you said documentation is missing. I guess this will require some non trivial effort and I really encourage you to dedicate enough time and energy to it. After all that is the contact point with the public and we want to keep quality there as high as possible. We might also do that in a separate PR. Actually I'd probably do so. I'd also deploy develop
branch documentation once that PR is merged. 😏
I have included above comments. I will now move on to implement some unit tests. As discussed, the documentation will be part of a separate PR. |
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 finally found time to go through this PR. Although I left many comments, they are almost all minor. Those who are not are mainly about amplification comments or the DRY principle. When implementing changes like you did in this PR one is very much tempted to duplicate and adapt, which is fine, provided that one refactors out duplications once done. 😉 Just do the proposed small adjustments, I am confident we are ready to merge then.
I happened to find time to take care of this earlier as expected, and I implemented all requested changes. |
This PR is not final yet, as notably
I did some test-driven development for this one, so I started with the functional tests and implemented all necessary features to offer a second module for the sampler. My assumption was that it is rather unlikely that anytime soon, a third sampler module will be added. Therefore, I decided to minimize code duplication and reuse as much of the existing infrastructure as possible.
Notably, the difference between the two samplers is limited:
I created the PR on purpose before everything is done, to already get some feedback in case some of the design decisions are suboptimal.