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

Add notion of sampler #12

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add notion of sampler #12

wants to merge 3 commits into from

Conversation

luisfpereira
Copy link
Owner

Inspired by #9, this PR brings in the notion of sampler.

Notice this commit was initially done in 415cd8d.
Nevertheless, I believe this code is mature enough to merge (maybe after adding a notebook showing how this work?), whereas we still have a lot of work to do with #9.

Still need to address a couple of comments I've left in the code.

@gviga
Copy link
Collaborator

gviga commented Dec 26, 2024

Hi @luisfpereira,
I was looking at and testing your implementation and I have some comments.

  1. The major issue that I have is our choice to separate the notion of a Sampler and the NearestNeighbour Finder. I think we already discussed and if I remember correctly this is about the idea of a Continuous Sampler. However, I think that we need first to assume that a sampler is Discrete, and in this case, it is useful to have both vertices and indices as output, while separating the two can be expensive since a lot of methods naturally return both. I think a discussion on this needs to be made and maybe we will have a clearer idea after the implementation of other Samplers.

  2. Particularly to this issues, i have wrote a notebook to explain the sampling and it is clear that with the code as it is now, we cannot have access to both vertices and indices without explicitly recover them.

Besides that, it seems that everything is working,

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.

2 participants