Let's keep the theoretical debate open until we've agreed on the resampling behavior for the discrete space

please start the debate

Ok, so now that we have arrived at the subset concept and have a clearer picture: would you still consider the theoretical necessary? Isn't it just fine to say we have n subsets that are spanned by the constraints? Or maybe give a concrete example (ideally with concrete numbers spanning the space) where you think the terminology is inappropriate.

for me that was always sort of unconnected to the naming debate, but here how this property came to life:

the cardinality implementation already had such a property as its easy to calcualte the number of possuble subsets ad hoc + there are no other considerations for conti space.

When implementing for discrete spaces and batch constraint you realize that the number of subspaces is not sufficient to make a judgement (eg in dispatching logic). You actually need the number of subsets with at least M candidates, lets call it n_feasible_subsets

At the same time I did not want to implement n_feasible_subsets because it requires evaluating the size of each subset which would destroy generating them lazily.

So then I chose to make the name of the n_subsets more precise and give it a name that indicates its just an idealized number. I picked theoretical but am open to suggestions

On the other hand, and maybe thats what you're saying as well, we could just ignore this and say n_subsets is already sufficiently describing the property and doesn't need further precision naming at the small danger of a reader then not being aware of the mentioned subleties.

I can follow the logic and would like to share two thoughts:

1. I think you last paragraph summarizes it: in the end, it's just convention, right? We could say subsets are really just the sets that we are optimizing over, full stop. We've never explicitly said that there aren't potentially other sets you could build, which are however irrelevant to optimization problem. If we take this interpretation --> all good, right?
2. In my opinion, the actual core of the debate is still this thread that you closed. To me, the collection of subsets that you discard (and that makes the difference between actual and theoretical) should not be discarded in the first place – at least not without explicitly opting out by setting some flag like allow_recommending_duplicates_within_batch. Because as long as that (non-existing) flag is set to True, you can always create an M-sized batch even if there is only 1 actual candidate left in the feasible region. So assuming that flag existed (and which we should probably add at some point), the whole debate sort of become obsolete in my opinion

this PR does not implement or aim to implement the conditional duplicated sampling from batches which arises in the general recommendation context and not just for subspace-generating constraints, hence I closed that thread as I would much prefer a clean implementation of that potentially delicate topic

There is another obvious visual argument that should make you stop requesting to include more loosely-related changes into this PR:

Given that, this PR operates in the world where conditional duplicated sampling from batches is not present and aims to set up properties such that they work in the world this PR exists in

So in that world: is there still anything to discuss and should we rename the property? I already shared the minor disadvantage I see

Don't get me wrong, I don't want you do implement more stuff in here. I just mentioned it to underpin my argument: if 1) the duplication stuff can be clearly resolved later and 2) there is a valid interpretation of the term subset that works for us --> I think it's fine if we drop the hypothetical part here and then take care of the proper handling of duplicates in a follow-up?

• do you mean theoretical?
• if you vote to drop it we need a tie-break

sorry, yes, I mean the theoretical. Summon your tie breakers :D

@AVHopp @kalama-ai need a tie breaking here between the choices

• rename n_theoretical_subsets to just n_subsets
• keep n_theoretical_subsets

explanation why theoretical was added in the first place here

I have a suggestion for the shuffle argument here and in the corresponding discrete case. What I dislike about the method signature is that we have two seemingly independent arguments which are however coupled in reality (see your docstring for shuffle). This always opens the door for silent "errors", i.e. when someone provides both flags as True. We've had this countless times in other places and I urgently want to avoid these designs in the future (a la avoid invalid/meaningless configs by design).

For this situation here, we might have a very simple answer, which even simplifies the entire thing: as far as I can tell, there is no use case where we'd actually require a deterministic order, right? Also, I don't think there is a canonical default order – after all, this depends on the (arbitrary) order of constraints etc. So why not simply drop the argument altogether and just make shuffle True by default?

as discussed: awaiting commit with suggestion

So this is my suggestion: eab2277

Note that it does a few things, in fact:

• Drops the shuffle argument and thereby resolves the problematic combination of arguments
• Simplifies the internal logic accordingly
• Make the entire thing more readable by separating into three logical steps: 1) apply per constraint 2) construct iterator according to input flag 3) assemble output from iterator

One important thing to note: Technically, this logic is not 100% equivalent to the previous one, since this no longer results in a flat uniform sampling but rather aggregated over groups. However, there is currently no situation where we would need a truly uniform sample, and the proposed implementation comes with significantly reduced code. If you say that uniformity is important, I think it's still possible to meet halfway, but implementing only parts of the changes.

hmm. wonder why you're glancing over this uniformity so fast, isn't it important?

1. If its not uniform, its in some sense biased, even if we don't explore for now in detail how its baised. Being biased seems bad to me
2. sample_subset_masks from discrete.py uses islice to select the first entries from the iterator. If its baised it will not guarantee a proper draw, I think it will even be a very badly structured draw, E.g. if group 1 shuffles to [B, A] and group 2 to [y, x], you always get (B,y), (B,x), (A,y), (A,x) — never e.g. (B,y), (A,x), (B,x), (A,y) - which doesnt make islice reasonable anymore. I remember that I specifically wanted to avoid this situation during development.

There is also a minor de-provement because infeasible samples are not removed from the pool anymore as before but appear now again

Ok, how about now? Added two more commits:

1. Restored the original logic but kept the shuffle change and one logic simplification
2. Used existing numpy helpers for the flat index computation

ok I am happy to keep the simplification in select_via_flat_index regardless (although I dont get why you removed the usually rated as helpful Examples/Notes)

But please take a look what your combined changes to the logic now achieved... there is almost no difference anymore. In particular nothing has become substantially shorter or simpler as claimed for your original reason to propose this change... so why do we not simply keep that tiny kwarg shuffle (even if only one of its possible values is used in the current code base)?

imo "improvements" like these are not even worth debating about...

also ngl I prefer the old version (left) here

as its more readable and uses the total variable which is now not removed