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

filtering/reconnecting #18

Merged
merged 18 commits into from
May 31, 2024
Merged

filtering/reconnecting #18

merged 18 commits into from
May 31, 2024

Conversation

brauliorivas
Copy link
Member

BEGINRELEASENOTES

  • Filtering/reconnecting particles is a critical task for dmX. So, in this PR, I'm implementing a basic filtering/reconnect function that given a function that determines if a particle should be filtered out, reconnects the particles again.

ENDRELEASENOTES

js/main.js Outdated
@@ -50,7 +50,7 @@ const mouseUp = function (event) {
isDragging = false;

// console.time("drawAll");
drawAll();
drawAll(parentLinks, childrenLinks, infoBoxes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stupid question from my side, where do these inputs come from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry, these inputs come from main.js.

@brauliorivas brauliorivas marked this pull request as ready for review May 14, 2024 22:28
@brauliorivas
Copy link
Member Author

Hey @kjvbrt and @tmadlener, could you check my filtering function? As previously discussed, an external function determines whether to filter or not a particle. And then reconnected by using the reconnect function in js/menu/filter.js. I have added a toggle for simplicity but it's temporary.

@brauliorivas brauliorivas changed the title filtering/reconnecting WIP filtering/reconnecting May 15, 2024
@tmadlener
Copy link
Contributor

I am rebasing this to potentially pick up the preview functionality.

}
}

function reconnect(criteriaFunction, parentLinks, childrenLinks, particles) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure whether this is necessary yet, but just for the future; Can this be written in a way where there are no parentLinks? We will probably need that e.g. for the ReconstructedParticle and I am wondering whether we should already spend the time now to prepare that?

@brauliorivas
Copy link
Member Author

Hey @tmadlener @kjvbrt, I started working on more advanced filtering for MonteCarlo Particles (as task for week 1 of the proposal). It's a bit buggy, but I want to share how is it going. Here is a small demo.

8eadf647-25e3-41ec-b2be-c97043fb2e95

@tmadlener
Copy link
Contributor

Very nice. Since you have the panel now already. Can I ask for one specific filter function which will should be straight forward to implement (from a technical point of view), but will probably require a bit of "leg work" from your side.

The edm4hep::MCParticle has two bitfields: generatorStatus and simulatorStatus. For the simulatorStatus we provide a few bit values that are reserved for a specific meaning:

https://github.com/key4hep/EDM4hep/blob/997ab32b886899253c9bc61adea9a21b57bc5a21/edm4hep.yaml#L294-L301

And then there is also some convenience functionality to check wheter a specifc bit is set:

https://github.com/key4hep/EDM4hep/blob/997ab32b886899253c9bc61adea9a21b57bc5a21/edm4hep.yaml#L306-L321

Would it be possible to have a filter that allows us to toggle these to filter out MCParticles for which specific bits are set? I.e. Have one checkbox / toggle for each bit and combine them into a filter function? IIUC all the filter combiners, etc. are already in place and this would mainly boil down to writing a specific filter for checking the bits in the simulatorStatus bit field.

@brauliorivas
Copy link
Member Author

I have it now. In theory it should work, but the particles from the json example don't seem to have a simulator status from the ones described, so it's a bit difficult to test. Here's an image of the filters now.
Selection_001

@tmadlener
Copy link
Contributor

Rebasing this again to see if the preview fixes from #22 are picked up.

Copy link

github-actions bot commented May 23, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-05-31 14:15 UTC

@tmadlener
Copy link
Contributor

tmadlener commented May 23, 2024

Preview is working 🚀 However, for me it doesn't show the filter panel. Do I have to enable them somehow? It looks like the preview doesn't publish the status of the PR, but rather the one from the PR. I am not sure why, though.

For testing the filtering, it would be nice to have some unittests for that to see if things work as expected. In these unittests you can just create some dummy MCParticle hierarchy with the values that you want in the end. You could start from the existing json file and just use a few of them and just adapt them to enable testing. This should all be possible without ever having to go to the UI to display them (for testing).

@brauliorivas
Copy link
Member Author

Preview is working 🚀 However, for me it doesn't show the filter panel. Do I have to enable them somehow? It looks like the preview doesn't publish the status of the PR, but rather the one from the PR. I am not sure why, though.

Yeah, it's strange. I didn't expect this. I'll fix it.

For testing the filtering, it would be nice to have some unittests for that to see if things work as expected. In these unittests you can just create some dummy MCParticle hierarchy with the values that you want in the end. You could start from the existing json file and just use a few of them and just adapt them to enable testing. This should all be possible without ever having to go to the UI to display them (for testing).

Sure! In that case, I will wait for this PR and #20 to be merged, to add testing, because the testing setup is in #20 and this #18 has the function to be tested. So I think that after merging both PRs to main, I should create a separate PR to test filtering, and other PR to fix again PR previews, what do you think?

@tmadlener
Copy link
Contributor

I am quite happy with this. I think we can merge this, unless @kjvbrt has more comments.

@@ -0,0 +1,70 @@
import { Link } from "../../objects";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import { Link } from "../../objects"; ->
import { Link } from "../../objects.js";

@kjvbrt
Copy link
Collaborator

kjvbrt commented May 31, 2024

Otherwise it runs nicely. For next PRs we can think of showing only sim/gen status numbers which are used in the file.

@kjvbrt
Copy link
Collaborator

kjvbrt commented May 31, 2024

I run it again and found that the filter is reset when I move any of the elements.

@brauliorivas
Copy link
Member Author

I run it again and found that the filter is reset when I move any of the elements.

Sorry, I thought it wouldn't matter because it's just like a "prototype". But I'm going to make it work when moving too now.

@kjvbrt kjvbrt merged commit 87295d6 into key4hep:main May 31, 2024
2 checks passed
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.

3 participants