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.

Sign up for GitHub

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

Jump to bottom

Maintaining in sync and updating openPMD-standard #38

Open
shervin86 opened this issue Jun 15, 2020 · 45 comments
Open

Maintaining in sync and updating openPMD-standard #38

shervin86 opened this issue Jun 15, 2020 · 45 comments

Comments

@shervin86
Copy link

I would like to know who is in charge of maintaining the openPMD-standard fork in sync with the upstream.

We need the issue tracker also for that repository (currently missing).

Furthermore, where is the particle beam extension documented? Has this been proposed and integrated to the upstream?
@shervin86
Copy link
Author

I have noticed that @aljosahafner is working on an openPMD extension for photon ray tracing:
https://github.com/PaNOSC-ViNYL/openPMD-standard/blob/latest/EXT_PRAYTRACE.md
and I wonder in what it is supposed to be different from:
https://github.com/DavidSagan/openPMD-standard/blob/EXT_BeamPhysics/EXT_BeamPhysics.md

At a very first glance I can see that there are quantities already defined even if not with the exact same definition. For example why one should define "wavelength" instead of "energy"? I think that one should re-use as much as possible common definitions between similar fields.
Another example is replacing "deadAlive" with "particleStatus" as defined in the EXT_Beam_Physics.

@aljosahafner
Copy link
Contributor

Hey, good question. I think this was not present at the time of making the photon raytracing extension. It would be good to align everything with our modifications during the sprint as well. Some of the things with openPMD are still unclear to me, we can discuss some time soon.

@shervin86
Copy link
Author

Yes, it can be a good thing to do during the sprint. Anyway I'm a totally newbie with openPMD, I just noticed and reported.

@aljosahafner
Copy link
Contributor

I mean, during the sprint (which happened some time ago) I was using the openPMD extension that was published as D5.1. However, during the development it turned out that some modifications had to be made. They're visible here: https://github.com/PaNOSC-ViNYL/workshop2020/blob/master/demo/team3/ShadowToSimEx_notebook.ipynb line 6, saveShadowToHDF()

@aljosahafner
Copy link
Contributor

@shervin86 I suggest we schedule a short meeting where we can discuss this and the database, then the situation should become clearer about everything. @ejcjason are you fine with that?

@JunCEEE
Copy link
Contributor

JunCEEE commented Jun 19, 2020

@aljosahafner @shervin86 Yes, it would be good to have a short meeting and sort out the problems we need to solve. Maybe we can have a short meeting after the WP5 vtc on Monday?

I know LCLS people are working on it actively: https://github.com/ChristopherMayes/openPMD-beamphysics It would be nice if the PRAYTRACE and be merged with Beamphysics, thus the efforts can converge.

@shervin86
Copy link
Author

@aljosahafner may I suggest you to prepare a set of slides, comparing the different fields you defined in PRAYTRACE and those in the beamphysics extension? The biggest part of the work should be to understand what is in common, what is not, and especially why something in the beamphysics definition is not satisfactory to your use case.

@aljosahafner
Copy link
Contributor

I don't know if I will have time today (depends on the WP5 meeting length), but I will check a bit and post the differences here. After that we can discuss, ok?

@shervin86
Copy link
Author

ok. Don't need to rush, let's just setup a meeting when you have looked at the differences. We can do it tomorrow.

@aljosahafner
Copy link
Contributor

aljosahafner commented Jun 22, 2020 •
edited
Loading

I checked a bit now, what would be the point in merging the two? The one made for photon ray tracing is made with the specific application in mind and is already compatible with existing workflows in at least 2 codes in Oasys.
If you take a look here ( https://github.com/DavidSagan/openPMD-standard/blob/EXT_BeamPhysics/EXT_BeamPhysics.md ), there are many redundant fields and many of the fields defined in photon ray tracing are missing.

For example why one should define "wavelength" instead of "energy"?

It is common to use wavelength in synchrotron experiments, that's all...

@shervin86
Copy link
Author

Did you have time to make the detailed comparison? Can you please keep me in the loop?

@aljosahafner
Copy link
Contributor

Hey, I had no time yet, I will let you know as soon as I go a bit deeper.

@aljosahafner
Copy link
Contributor

Hey, I checked now. There aren't many differences in terms of physics and I am sure that photon raytracing is (will be) a subset of beamphysics extension. However, as all the openPMD things it is very poorly documented.

The way photon raytracing extension is done right now is according to the codes inside Oasys, so it is fairly straightforward to export data. I have prototyped a widget for Oasys which will do just that. There will probably be a dedicated Panosc widget panel which will include this one.

We would have to speak with the core openPMD team responsible for beam physics extension and then my photon raytracing extension could become a documented case for the application of beamphysics extension to photon raytracing. I hope this is clear enough what I mean. @ejcjason @shervin86 please propose the next steps.

@JunCEEE
Copy link
Contributor

JunCEEE commented Jun 30, 2020

@aljosahafner It's good that there aren't many differences. Could we then use openPMD-beamphysics API with this example to directly write the output of photon raytracing data of OASYS?

@aljosahafner
Copy link
Contributor

That's the goal, no? My question is how to reach it... What is the best way to end up with .write_oasys_raytrace() method in openPMD-beamphysics?

@JunCEEE
Copy link
Contributor

JunCEEE commented Jul 2, 2020

Thanks, @aljosahafner . I am not sure now, I always have the impression that the goal here is to merge the raytracing standard into beamphysics standard. Maybe we need the input from @CFGrote to clarify that.

Intuitively, if there is no conflict in definition, then the easiest way in my mind is to merge ray-tracing standard into beamphysics standard and use the unified beamphysics API. That's why I asked the above question.

But if the final goal is to have a subset standard under beamphysics, I can send an email to let the LUME people know this, but then I feel this is not really a converge. The tools are still separated, the standard is still separated. There is no need to put a subset raytracing API into the beamphysics repository in my opinion.

@CFGrote Could you please comment here? The next step is also not clear to me.

@shervin86
Copy link
Author

shervin86 commented Jul 6, 2020 •
edited by aljosahafner
Loading

PANOSC_v2.pdf
Dear all,
I just prepared a couple of slides with some very elementary comparison between the two extensions.
I think that the best way to proceed is:

  • to clearly define what is missing in the beamphysics extension or which fields would be very difficult to use
  • find the beamphysics API and add what we might need (prepare a branch for a PR)
  • test in Oasys the new API
  • contact the beamphysics authors and organize a meeting to discuss our proposed changes (make a PR in parallel)

@aljosahafner
Copy link
Contributor

Hey, thanks for the slides and most importantly for proposing an action plan!
We should discuss in person about it (is there the regular meeting today?). For discussion I propose the following agenda:

  • Duplicated/calculated entries, why not? Why yes?

  • Naming convention (e.g. velocity, etc.) and conforming to it

  • Integration into a working prototype for Oasys, when?

  • Anything else

@aljosahafner
Copy link
Contributor

Hey, I have already defined the (absolutely) required minimal set of fields back in D5.1. This is enough to reconstruct the simulation at a given point and propagate it further. This could be a starting point...

image

@aljosahafner
Copy link
Contributor

aljosahafner commented Jul 10, 2020 •
edited
Loading

https://github.com/aljosahafner/openPMD-standard/blob/latest/EXT_BeamPhysics_PRAYTRACE.md

As per @shervin86 pdf:

  • position = position

  • direction = velocity (we can propose a change of name)

  • wavelength added as new field, since it's the most used out of energy and wavelength and neither is present

Work in progress... An equivalent has to be found for:

  • s- and p- polarised electric fields

  • s- and p- polarised phases (real and imaginary part)

@shervin86
Copy link
Author

I think it is ok. For the energy vs wavelength` I think that it is a bit tricky so, something should maybe be done at the API level to ensure that they are not both filled or enforce the consistency.

About the polarization, if I recall correctly they are defined w.r.t. a place of incidence. But in the openPMD format this is not defined.
Polarization could be defined w.r.t.:

  • global coordinate system
  • latticeElement local coordinate system
  • photon direction
    I don't know what would be the best, but it is important to have it in an unambiguous way

@aljosahafner
Copy link
Contributor

aljosahafner commented Jul 20, 2020 •
edited
Loading

@andrealorenzon is now also involved in this. We have came to a conclusion that making N groups for each particle, each having only scalar values for position, direction, etc. is not the best solution. We will proceed as before, therefore having one group and then each quantity having N x 1 vectors for each particle.

Fields that are absent in the beamphysics extension will simply be added.

@aljosahafner
Copy link
Contributor

aljosahafner commented Jul 22, 2020 •
edited
Loading

Hey, we have produced something much better than initially submitted as D5.1. It is just the minimal set of quantities to reconstruct the E-field intensity and phase. It is a good ground on which an API/link will be built. The final hierarchy is the following:

@shervin86
Copy link
Author

I am wondering why have you removed stuff from https://github.com/DavidSagan/openPMD-standard/blob/EXT_BeamPhysics/EXT_BeamPhysics.md ?
Shouldn't we try to merge?

Another thing, since the description could be valid also for high energy beam physics (protons in an accelerator for example), then wavelength is not really much usefull. Since a low energy is just a matter of conversion... I would be in favour of energy instead of wavelength.

@aljosahafner
Copy link
Contributor

I think X-ray raytracing specific "standard" must have wavelength, as everyone uses this, otherwise no need to call it X-ray raytracing standard.

Regarding the missing fields, do you think it's better to leave all the redundant fields in the document?

@CFGrote
Copy link
Contributor

CFGrote commented Sep 23, 2020

ok, found it.

so here are my review comments for https://github.com/aljosahafner/openPMD-standard/blob/latest/EXT_BeamPhysics_PRAYTRACE.md:

  • the records under Scope/Required are not up to date anymore, right? Attributes now details how you want to organize the data. You list all your records (momentum/, photon[S,P]Polarization[Amplitude,Phase], etc under Attributes. Remove the records from Scope/Required and remove the Attributes headline.

  • Your proposition means that you are not storing data by particle any more but by property and the particle id is then encoded as the column index in each field.
    I think this is fine because this is how shadow and most raytracing codes store data internally and it would make IO to and from the backengine faster as it avoids transposing and splitting/concatenating a lot.

But this clearly means we deviate from the upstream and David Sagan's (LCLS) extension draft https://github.com/DavidSagan/openPMD-standard/blob/EXT_BeamPhysics/EXT_BeamPhysics.md in an incompatible way.

I think we should move on with our proposal as we have good reasons, but we must not call it beamphysics anymore but raytracing instead.

  • Why storing the min and max? are they automatically updated when the data changes? does this mean any performance gain over calculating them on the fly?

  • If we do this (deviate from beamphysics), we may as well do it with the NRaytracing extension and maybe even combine both into one, but that's only a nice-to-have.

@shervin86, @ejcjason , any comments?

In any case, this is really good work, thanks a lot @aljosahafner !

@aljosahafner
Copy link
Contributor

Hey, when is the meeting?

@shervin86
Copy link
Author

shervin86 commented Sep 29, 2020 via email

@shervin86
Copy link
Author

Shall we do today at 2pm ?

@CFGrote
Copy link
Contributor

CFGrote commented Oct 2, 2020 via email

@shervin86
Copy link
Author

To be more effective the best would be to have a first meeting only @aljosahafner @mads-bertelsen and me, and then review the outcome of the discussion with you @CFGrote

@aljosahafner
Copy link
Contributor

Hey, I was away on Friday, we can meet anytime this week.

@shervin86
Copy link
Author

shervin86 commented Oct 14, 2020 •
edited
Loading

Here's the outcome of our meeting (Alijosa, Mads, Shervin)

Particles' variables: one entry per ray

  • position (x,y,z)
  • direction (x,y,z) -> x^2+y^2+z^2 = 1
  • nonPhotonPolarization (x,y,z) [optional] -> this is needed for neutrons
  • wavelength (SCALAR)
  • rayTime (SCALAR) [optional] -> in neutron simulation it is the time w.r.t. emission of the neutron
  • weight (SCALAR) [optional] -> this should be included already in the openPMD standard
  • sPolarizationAmplitude (x,y,z) -> only for photons
  • pPolarizationAmplitude (x,y,z) -> only for photons

Variable of the group of particles:

  • mass (SCALAR) -> this can be automatically set by the API if the PDG ID is known
  • particleSpecies -> write the PDGID as a string

The minValue and maxValue per record should be made mandatory. It is easy to calculate them when generating the openPMD file, and it can save a lot of CPU when inspecting the created file to know if it fits the user needs.

API for the extension

The code use for the openPMD I/O component could be used as baseline for the development of the C++ API of this extension. It could be a good idea to make the Python API by binding the C++ API using PyBind11. The advantange w.r.t. a native python API is the fact that developments are done only in one place and there is minimum effort to keep the two APIs in sync.

It is a good idea to find a way to include the simulation configuration and parameters in the openPMD file. In the case of McStas, it means including the entire instrument file. This ensures the possibility to reproduce the content of the openPMD file, and to understand it.

Additional variables related to the coordinate system

In order to allow the use of the openPMD file in other simulation softwares, it is necessary to ensure that the coordinate systems used are the same or that a conversion is performed by the API.
So it is mandatory to provide such information.
We have identified the following:

  • direction of gravity (x,y,z)
  • horizontal coordinate (x,y,z)

This part might need some more thinking to ensure that a coordinate transformation is always possible from the coordinate system of the openPMD file and that of the software reading it.

@shervin86
Copy link
Author

CAVEAT:
we need to improve the documentation of the numParticles variable. This should represent the number of effective particles in the file, that might be different from the extent of the dataset.
When an openPMD file is created, the number of saved rays might not be known.

This can happen if it is chosen not to save all simulated rays, e.g. only alive rays are saved, or only some in an energy range, or only those reaching a particular position along the beam line.

So in the documentation this should be made more clear and the API should be using this variable when reading rays from the file.

@shervin86
Copy link
Author

shervin86 commented Oct 16, 2020 •
edited
Loading

@aljosahafner can you please push your developments in a branch in this repository?
I'm going to create a repo for the C++ API and pushing something into it:

https://github.com/PaNOSC-ViNYL/openPMD_raytrace_API

@aljosahafner
Copy link
Contributor

image
I cannot open a pull request... Now I remember this is also the reason why I have my own repository.

Please add also S- and P- polarized amplitudes to the particles variables.

@shervin86
Copy link
Author

Ok, I'm slowly making refactoring the base class and making the documentaiton.
Here's a preview:
https://panosc-vinyl.github.io/openPMD_raytrace_API/classrays.html

@aljosahafner
Copy link
Contributor

Hey, I started updating, the current version is seen here:
https://github.com/PaNOSC-ViNYL/openPMD-standard/blob/latest/EXT_BeamPhysics_PRAYTRACE.md

@shervin86
Copy link
Author

Ok, so the basic structure of the API is drafted.
The main page of the documentation is here: https://panosc-vinyl.github.io/openPMD_raytrace_API/index.html

Make sure to pick up the devel branch

@shervin86
Copy link
Author

@aljosahafner
can you please let me know your opinion about the API? Are the classes and methods easy to understand and do you think that their usage can fit in OASYS?
Before fixing everything and polishing it, I would like to make sure it is well structured.

@aljosahafner
Copy link
Contributor

aljosahafner commented Oct 27, 2020 •
edited
Loading

Oasys is done completely in Python, so C++ is not very useful, but something similar will be made for Python as well.

I guess this is the list of implemented methods/classes: https://panosc-vinyl.github.io/openPMD_raytrace_API/namespaceraytracing.html

@shervin86
Copy link
Author

There are two classes that are the public API:

@aljosahafner
Copy link
Contributor

Hey, phase is missing. Shadow (the code in Oasys) has some sort of algorithm to calculate the phase as well...
Here are all the fields necessary (including the ones we discussed for neutrons and gravity/horizon directions): https://github.com/PaNOSC-ViNYL/openPMD-standard/blob/latest/EXT_BeamPhysics_PRAYTRACE.md

@shervin86
Copy link
Author

I think we can close this issue.
Can @JunCEEE @CFGrote @aljosahafner let me know if there is an specific branch in this repo we want to keep?

We should remove all useless branches and clean the repo.
I would like to have latest and upcoming in sync with the official openpmd branches.

@aljosahafner
Copy link
Contributor

I agree with you. We can keep only the PR branch raytrace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@JunCEEE @shervin86 @CFGrote @aljosahafner