Exclusive jet constituents #137
Conversation
src/fastjet/_generalevent.py
Outdated
| ), | ||
| behavior=self.data.behavior, | ||
| ) | ||
| outputs_to_inputs = ak.Array( |
There was a problem hiding this comment.
Up until this point, the basic code of exclusive_jets_constituents and exclusive_jets_constituent_index is the same. I wonder if exclusive_jets_constituents should call exclusive_jets_constituent_index or will this cause unnecessary overhead?
A similar situation applies to constituent_index and constituents. Clearly, in that case, it was chosen to do it this way. So, this approach is probably the right way to go.
If it turns out it is useful to minimize code duplication, we can deal with it more systematically in a follow-up PR.
|
@lgray @chrispap95 thumbs up if this PR looks fine to you. If there are no complaints, I will plan to merge it. |
src/fastjet/_generalevent.py
Outdated
| np_results = self._results[i].to_numpy_exclusive_njet_with_constituents( | ||
| njets | ||
| ) | ||
| off = np.insert(np_results[-1], 0, 0) |
There was a problem hiding this comment.
This numpy operation resembles the code that was causing problems with inclusive_jets(). The code was working fine but when tested at scale it was giving seg faults sporadically. In the end, we realized that the offset array needs to be len+1 in the C++ code and here we just get the array: off = np_results[-1]. Now, I realize that I didn't remove all those parts :/ I am fine with keeping this code as is but keep in mind this problem. And we might want to remove all those lines in the future (including the code outside of this PR).
There was a problem hiding this comment.
I think it would be best to sanitize memory access before we merge this. @chrispap95 can you summarize a list of checks / fixes that should be addressed before we merge this?
There was a problem hiding this comment.
The fix is to change the allocation size for the offsets array in C++ to {len+1} from {len} and simplify python code from off = np.insert(np_results[-1], 0, 0) to off = np_results[-1]. Also, it would be fantastic if this can be done as well for any methods that I forgot to patch. @rkansal47 let me know if you have trouble doing this.
There was a problem hiding this comment.
Basically, all np.insert() need to go away.
There was a problem hiding this comment.
OK, I believe I've fixed all of them (needed to just insert the 0 in C++ as well).
|
I think there is a typo |
|
Indeed, I was just fixing that :), still some tests failing though. |
Adding the
exclusive_jets_constituent_indexandexclusive_jets_constituents(for a specified number of jets) functions, which retrieve particle constituents of exclusive jets similar to theconstituentsandconstituent_indexfunctions.This partially solves #100, but future PRs may want to add the
dcutarg and implementations for exclusive subjets as well.