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

Idea: move particle search functionality to a new ParticleDB class #277

Open
eduardo-rodrigues opened this issue Nov 12, 2020 · 4 comments
Labels
⚙️ enhancement New feature or request
Milestone

Comments

@eduardo-rodrigues
Copy link
Member

The idea has been around in our heads for a while. It came out again in discussions in #263. Let's have this as a possible API for version 1.0.

For reference: indeed the present Particle class is both the class for particle properties and for searches. For example Particle.from_pdgid() returns a Particle instance whereas Particle.findall() returns a list of Particle instances. While very powerful, this may be confusing at first. One could split functionality between a slightly different Particle class and a Particles/ParticleTable class.

@eduardo-rodrigues eduardo-rodrigues added the ⚙️ enhancement New feature or request label Nov 12, 2020
@eduardo-rodrigues eduardo-rodrigues added this to the 1.0.0 milestone Nov 12, 2020
@HDembinski
Copy link
Member

Sounds good. Why not make find() and findall() functions on the module-level? Why attach them to a class? I think the fundamental question is whether you want to expose the underlying database to the user or keep it an implementation detail. If you want to expose the database and make it easy for users to change the data source of the database, then it would be better to expose a ParticleTable class or ParticleDB.

I am not sure whether this is a good idea. I like the simplicity of particle and I trust it to use the most recent world average data when I import the most recent version. I don't want to use outdated masses and life-times and I cannot imagine people will. So I think it is a good choice to not expose the DB to the user as it is now.

@henryiii
Copy link
Member

henryiii commented Mar 26, 2021

So I think it is a good choice to not expose the DB to the user as it is now.

The ability to add particles, modify particles, or load a new table is a key component to Particle's original design, and is used in several applications, like DecayLanguage, and in at least one analysis. There are a variety of reasons you may want to load specific older data, or modified data, or add particles.

Why not make find() and findall() functions on the module-level?

Because Particle was explicitly designed to be simple to use in a REPL. You can access all the functionality with just a from particle import Particle. It also lets you subclass Particle, and still use find, while a module level function would still return the original Particle class. Also, this was designed to provide equal access to the Particle class and the PDGID class; why does find return a Particle and not a PDGID? Unless you really want particle.particle.find...

If you are not in a REPL, find and findall are often not the best tool to use, it's better to load from PDGID or a constant. They were explicitly designed for REPL usage.

ParticleTable class or ParticleDB

I like those names better than Particles, on reflection, easier to distinguish. Really, this would be the "correct" API:

from particle import ParticleDB
pdb = ParticleDB()  # default particle table
p = pdb.find(...)

However, this requires a user to learn about Particle databases, to keep their own state (pdb above), and to work with more classes, all of which are counter-productive to the goal of Particle to provide fast, simple, and interactive access to the pdg data. All this requires more training and documentation, extra space in each user's brain, extra lines of code. Most users do not need to load special data, so this extra manipulation just gets in their way. Also, this would allow the creation of multiple states, and the original design did not allow multiple particles to share the same PDGID; so this would give you apparent freedom to do things not allowed by design. This might be changed now, or might be changeable in the future.

The current design is a balance between the two uses - you have a simple way to load particles, and an "advanced" way to modify, replace, or extend the particle table.


For a proposal to move forward, I would recommend the following: Add a ParticleDB class as seen above, and refactor the current _table attribute to hold a default "latest" ParticleDB table. Then .find* class method shortcuts would forward to the cached instance. We could remove the load table functionality on Particle; anyone wishing to use an explicit version of the particle table could use the API shown above; the shortcuts would only be for people who only want the most recent world averages.

This of course would need to happen after thought is put into what it means to compare two particles that were loaded from different ParticleDB's.

@eduardo-rodrigues
Copy link
Member Author

I'm on the fence here and will need to think carefully, not Friday at the end of the busy week ;-). In any case my really general comment is that we should not forget that so far we hardly ever had a complaint on the API. I would conclude from there that it's pretty good. In other terms, whatever we do should not totally break the way we have people working with the package, as that's likely not so welcome.

On general grounds I always liked very much the fact that the user does not have to care with the DB/files, which are behind the scenes. We provide the latest info (and the user can go back in history if they so want) with a powerful API, and that's paramount. Now, if we want to have a, separate, way to "play" with the DB ...

OK, more when I get to think about the future of Particle properly :-). Have a good weekend.

@henryiii henryiii changed the title Idea: move particle search functionality to a new Particles class Idea: move particle search functionality to a new ParticleDB class Mar 29, 2021
@HDembinski
Copy link
Member

Because Particle was explicitly designed to be simple to use in a REPL. You can access all the functionality with just a from particle import Particle. It also lets you subclass Particle, and still use find, while a module level function would still return the original Particle class. Also, this was designed to provide equal access to the Particle class and the PDGID class; why does find return a Particle and not a PDGID? Unless you really want particle.particle.find...

Ok, that's an explanation for the choice, but this is designing for the developer and not designing for the user. Users have no need to derive from Particle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants