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

Create new types of collections #25

Closed
wants to merge 17 commits into from

Conversation

brauliorivas
Copy link
Member

BEGINRELEASENOTES

  • New types of collections. The collections to be implemented are Cluster, Reconstructed Particle, Particle, Vertex, and Track.
  • The information about each collection has been obtained from EDM4hep yml file.

ENDRELEASENOTES
The task for GSoC week 1 is almost done. We only need to finish previews. So, I have started to do the task for week 2.

Copy link

github-actions bot commented May 29, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-06-11 12:48 UTC

Comment on lines 5 to 9
"AllMuon": {
"collID": 12,
"collType": "edm4hep::ReconstructedParticleCollection",
"collection": [],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as an FYI: this seems to be a subset collection, i.e. the actual data live in another collection and this just has "pointers" into that collection. Looking at this bit of JSON here, I think edm4hep2json does not yet handle this case properly (unless you have cut quite a bit of the original JSON).

Copy link
Member Author

Choose a reason for hiding this comment

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

the actual data live in another collection and this just has "pointers" into that collection.

Ohh thanks. I didn't know that.

Looking at this bit of JSON here, I think edm4hep2json does not yet handle this case properly (unless you have cut quite a bit of the original JSON).

Yeah, I think it is about edm4hep2json. I took the first collection without cutting. I will change it.

index.html Outdated
Comment on lines 20 to 22
<p>¡Welcome to <span id="logo">
<p>¡¡¡Welcome to <span id="logo">
<span id="logo-d">d</span><span id="logo-m">m</span><span id="logo-x">X</span>
</span>!</p>
</span></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be removed again, now that we know that the preview is working, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I've removed them now.

},
"Jet": {
"collID": 13,
"collType": "edm4hep::ReconstructedParticleCollection",
Copy link
Contributor

Choose a reason for hiding this comment

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

Another remark here. The edm4hep::ReconstructedParticleCollection will start to look differently starting with version 1.0 of edm4hep. In principle the version of edm4hep that was used to write the file should also be in the json file, so you should be able to check which version to laod in the end.

In practice, I would do the following for now:

  • Ignore the particleIDs and particleIDUsed in the edm4hep::ReconstructedParticle for now (it seems like you are not yet connecting the loaded entities, so this is more of a comment for you to keep in mind)
  • Foresee a version check in the loading of the different types and / or a possibility to change the loading dynamically depending on the file contents

@brauliorivas brauliorivas marked this pull request as ready for review June 5, 2024 23:21
@brauliorivas
Copy link
Member Author

Currently, not only types are created. But also the loading functionality.

  • Foresee a version check in the loading of the different types and / or a possibility to change the loading dynamically depending on the file contents

And, there is also a version check, so these types only work with a set of versions. Also, the particle properties are assigned according to the data in the JSON.

Comment on lines +4 to +5
static MIN_VERSION = "0.7.0"; // may vary per type of particle
static MAX_VERSION = "1.0.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

TL;DR: I like this approach, but we might have to come back to it in the future to make it a bit more flexible, and I just want to plant this information in your head, so that you can also start to think about it in the background :)


Nice. I think this goes into the right direction. I have checked the handling of the versions a bit below and I think it's OK. I have one more design question here, what would happen if e.g. the Cluster changes in EDM4hep in version 1.2.3 (which one doesn't actually matter)? How would you then be able to work with "old" clusters and "new" clusters? Have you thought about that already?

Just to give you some of the requirements here:

  • Different versions of datatypes might have different data members
  • Different versions of datatypes might have different relations and vector members
  • Different versions of datatypes might even have different names (although I think we can ignore this for now).

So what we would probably need is some way of reading the data members and relations depending on the version. One way that could be done already now would be to simply create a new Cluster class (with a slightly different name) and then use the mechanism that you already have in place now. The other option would be to do some dynamic handling at least of the reading part depending on the version.

For displaying the potentially different information in the end, I am not yet sure how to handle this best, but for starting we could probably just have the set of all members that are in common (if there are changes).

Copy link
Member Author

Choose a reason for hiding this comment

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

what would happen if e.g. the Cluster changes in EDM4hep in version 1.2.3 (which one doesn't actually matter)?

Right now the most simple way that I can think of, is to create multiple classes of Cluster, and choose the one that fits the version.

How would you then be able to work with "old" clusters and "new" clusters? Have you thought about that already?

Well, almost all classes should have general draw, getData, etc methods that each version has to implement.

One way that could be done already now would be to simply create a new Cluster class (with a slightly different name)

Yeah, exactly!

The other option would be to do some dynamic handling at least of the reading part depending on the version.

There is something similar already

But yes, thinking of a way to load the particles and manage them across dmx independently of the version is a must. I will design this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't aware of the dynamic loading bit, thanks for pointing it out.

Another comment that adds a bit more information on how EDM4hep "behaves" (or should behave) when reading older versions. Users only ever see the latest version in memory, and also the interface they get will always be the latest version. All the schema evolution from the older version to the newer version happens before users actually see the data. That is why I would try to keep a similar behavior also here, i.e. once the data has been read the classes are at their latest version.

Having read through your comment, it might even be OK for now to just follow your approach, since draw and getData are already things that can be overriden, so we should in principle be quite flexible in case there is no immediately better design.

tmadlener
tmadlener previously approved these changes Jun 10, 2024
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good to me. @kjvbrt anything still from your side? Otherwise I would merge later today. Ah sorry, wrong PR.

@tmadlener tmadlener dismissed their stale review June 10, 2024 11:57

wrong PR

@brauliorivas
Copy link
Member Author

brauliorivas commented Jun 10, 2024

Hey @tmadlener, I think we should continue with #36 instead. If you want, you can close this PR.

@tmadlener
Copy link
Contributor

superseded by #36

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