ComputationalRadiationPhysics / picongpu

Performance-Portable Particle-in-Cell Simulations for the Exascale Era :sparkles:
https://picongpu.readthedocs.io
Other
694 stars 218 forks source link

Should we rename namespace picongpu::openPMD ? #3390

Open sbastrakov opened 3 years ago

sbastrakov commented 3 years ago

It causes to use openPDD API as ::openPMD:: and for those who are unaware this may cause trouble e.g. when copying from openPMD API examples

sbastrakov commented 3 years ago

To clarify it is also (in my opinion) difficult to quickly read the code that uses both ::openPMD:: and openPMD, often in the same expression.

So I would prefer if picongpu::openPMD would become smth like picongpu::openPMDHelper.

ax3l commented 3 years ago

Potentially. Also I recommend doing this after #include:

namespace io = openPMD;

which makes it unique, too ;D

sbastrakov commented 3 years ago

I think while there are other IO libraries in use that would be a weird alias to use

sbastrakov commented 3 years ago

I mean, weird ourside of the openPMD "land", as one reading PIConGPU code would need to know io means oprnPMD. And having at the same time picongpu::openPMD does not become less confusing then imo

psychocoderHPC commented 3 years ago

I for changing picongpu::openPMD to picongpu::openPMDHelper. namespace io = openPMD; is also possible but I think we do not have so many openPMD interactions in the code so being a little bit more verbose and writeing openPMD::* is better.

franzpoeschel commented 3 years ago

Axel's suggestion would mean to rename the original openPMD namespace (i.e. the one by the openPMD API) as io? I agree that this might be confusing, I would leave that namespace untouched. What about renaming picongpu::openPMD as picongpu::io instead? That would make sense imo since the openPMD plugin is in fact PIConGPU's IO plugin. I don't really like openPMDHelper, things like *helper or *detail always sound too much like it's some little namespace that you're not supposed to be touching, while in reality it's the complete plugin…

ax3l commented 3 years ago

Axel's suggestion would mean to rename the original openPMD namespace (i.e. the one by the openPMD API) as io?

Not sure if we mean the same thing, probably. This is just an example of how import numpy as np and import matplotlib.pyplot as plt looks in C++ inside user code. I use this consistently in examples in the docs because it makes code nice and concise. There is no need to follow it, it's just a short-hand and my ;D indicated the half-joke.

I agree with René's suggestion. I just mentioned this because namespace aliases are pretty and too seldomly used :)

franzpoeschel commented 3 years ago

No, this is just an example of how import numpy as np looks in C++ inside user code. I use this consistently in all examples in the docs because it makes code nice and concise. There is no need to follow it, it's just a short-hand.

Yeah, but this would be the effect of it, right? Renaming openPMD's openPMD namespace as io within PIConGPU, so PIConGPU can use its own picongpu::openPMD namespace without having things clash? No issue with namespace io = openPMD in general, but introducing our own picongpu::openPMD namespace afterwards is asking for trouble :D If we want to rename things, picongpu::openPMD is the better candidate imo

ax3l commented 3 years ago

Yep, I think renaming it to "...detail" is not super pretty but ok and already used throughout the code base.

franzpoeschel commented 3 years ago

If I'm interpreting the discussion correctly, @sbastrakov and @psychocoderHPC 's objections were to having namespaces picongpu::openPMD and io = openPMD. I would go for picongpu::io and leaving openPMD untouched, but if picongpu::openPMDHelper is preferred to keep things in the same style, I'm fine with that too ;)

sbastrakov commented 3 years ago

Sorry, when creating this issue and discussing it before I was confusing two things. So there is picongpu::openPMD namespace for the openPMD plugin. And it was also (by coincidence?) used for the ParticlePatches class. And I meant to rename that one to openPMDHelper, and maybe add some more stuff there, not to rename the plugin namespace, which is how everyone understood that probably.

But now I've figured the ParticlePatches class is not used and made #3419 to remove it.

sbastrakov commented 3 years ago

And for the openPMD plugin, maybe it could become picongpu::plugins::openPMD (currently some plugins have that plugins in between but most do not), then I think it would be much harder to confuse with openPMD API.

franzpoeschel commented 3 years ago

If we rename picongpu::openPMD as picongpu::plugins::openPMD, this won't really solve the issue with having to use ::openPMD for the original openPMD namespace though?

sbastrakov commented 3 years ago

Yes, it won't really for usage of openPMD API inside the plugin, which is most of the usage right now.