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

WorkflowManager #104

Closed
wants to merge 19 commits into from
Closed

WorkflowManager #104

wants to merge 19 commits into from

Conversation

steinnymir
Copy link
Member

Here is a very basic implementation of the workflow manager as I described in the last meeting.

A notebook shows the basic principle of how this works and how a workflow step needs to be defined.

There is clearly more functionalities to be added, and I will continue adding some workflow steps in the coming days.

I'll make it a proper pull request once a few more workflow steps are defined.

@rettigl
Copy link
Member

rettigl commented Feb 23, 2023

Do I understand this correctly, that you intend this workflow manager to replace the current processor?
I see two difficulties:

  1. The workflow steps are executed only at the end. How should we handle steps that generate parameters for the functions to be applied to the df, like e.g. the energy calibration fitting, distortion correction, etc? One could still define them as (static) methods in the workflow manager, and only have them write their results into the workflow queue. This would however remove some of the flexibility.
  2. How do you get parameters from the configuration into the workflow steps, e.g. default calibration parameters, etc.? My goal when setting up the current workflow steps in the processor was to reduce the exposure of config parameters, etc. to the user to an absolute minimum, to lower the entrance barrier, and to make it as convenient to use as possible. This off course goes to some extend at cost of flexibility...

Generally, the energy calibration function you wrote is essentially the same as in the energy calibrator module:

def tof2ev(

i.e. we should try to consolidate this, and not duplicate code...

@steinnymir
Copy link
Member Author

Thank you @rettigl for looking into this. I'll try answer your questions in a few points

  1. this is intended as an automatic pipe, which runs all provided methods with the given parameters with no need for user input. This is necessary if we want this to run as an automatic correction system (even though only on the long run). Also, makes it the simplest approach for users, as they need only call a list of trnsformations, which can be pre-defined, making for example a standard correction pipe for a given beamtime/experiment.
  2. The definition of the parameters should be a separate problem. Each user should have the freedom of using the tool that they prefer to do this. However, we of course will provide one (python based) method to determine them for each correction method we provide. The structure of these is not yet defined, but could be presented in jupyter notebooks as examples. This also gives a space to properly describe how these transformations work, should a user be interested. The generated parameters can be then stored in a json/config file
  3. this json/config currently these methods need explicit parameters, but this can easily be switched to picking values from a json/config file instead. These could be defined as in point 2, or kept as fixed values provided by the beamline (for corrections which don't change over an experiment)
  4. I know the function is very similar to what you added, but if you look at the structure of it, (class) it does not have the same implementation. Obviously, we should not keep processor and workflow at the end, and the corrections you imported from MPES will need some adaptation for the new system.
  5. I am also planning to make a workflow_step generator function, which passing it a function, returns a subclass of WorfklowStep which runs the given function. THis could both simplify transition of existing mehtods, and add the possibility of user generated transformations

I hope this wall of text answers some of your concerns, and keep in mind this is still work in progress 😄

@zain-sohail
Copy link
Member

as discussed, I will try to include #97 in this (just for my reference)

@zain-sohail
Copy link
Member

Any updates on this?

@steinnymir
Copy link
Member Author

This PR could probably be closed, as I have restarted from an other more recent branch, to avoid needing to merge in all changes made in the mean time.

Other than that there is no real update so far, I hope to have something soon though.

@steinnymir steinnymir closed this Aug 15, 2023
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.

3 participants