PaNOSC-ViNYL / ViNYL-project

This repository keeps track of tasks, milestones, deliverables of workpackage 5 in panosc.
Apache License 2.0
5 stars 5 forks source link

Maintaining in sync and updating openPMD-standard #38

Open shervin86 opened 4 years ago

shervin86 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

aljosahafner commented 4 years ago

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

aljosahafner commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

aljosahafner commented 4 years ago

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:

aljosahafner commented 4 years ago

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 commented 4 years ago

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

As per @shervin86 pdf:

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

shervin86 commented 4 years ago

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.:

aljosahafner commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

ok, found it.

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

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.

@shervin86, @ejcjason , any comments?

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

aljosahafner commented 4 years ago

Hey, when is the meeting?

shervin86 commented 4 years ago

Shall we do tomorrow? Anytime in the afternoon. Or on Thursday, anytime after 10 am.

-- Shervin

Da: "aljosahafner" notifications@github.com A: "PaNOSC-ViNYL/ViNYL-project" ViNYL-project@noreply.github.com Cc: "shervin86" shervin@email.it, "Mention" mention@noreply.github.com Inviato: Lunedì, 28 settembre 2020 20:11:48 Oggetto: Re: [PaNOSC-ViNYL/ViNYL-project] Maintaining in sync and updating openPMD-standard (#38)

Hey, when is the meeting?

— You are receiving this because you were mentioned. Reply to this email directly, [ https://github.com/PaNOSC-ViNYL/ViNYL-project/issues/38#issuecomment-700197378 | view it on GitHub ] , or [ https://github.com/notifications/unsubscribe-auth/ABC4EJNQW7KQXLATPTFPEPDSIDGWJANCNFSM4N54T4RA | unsubscribe ] .

shervin86 commented 4 years ago

Shall we do today at 2pm ?

CFGrote commented 4 years ago

would work for me if my presence is needed.

On Fri, 2020-10-02 at 00:14 -0700, shervin86 wrote:

Shall we do today at 2pm ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. -- Carsten

-- Dr. Carsten Fortmann-Grote Scientific Computing Services Max-Planck-Institute for Evolutionary Biology August-Thienemann-Strasse 2 24306 Plön Germany

Room: 115 Phone: +49 (0) 4522 763 277 Email: carsten.fortmann-grote@evolbio.mpg.de

shervin86 commented 4 years ago

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 commented 4 years ago

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

shervin86 commented 4 years ago

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

Particles' variables: one entry per ray

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:

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

shervin86 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

There are two classes that are the public API:

aljosahafner commented 4 years ago

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 commented 2 years ago

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 commented 2 years ago

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