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

Proposal: Possibility to not save all parameters. #1014

Open
poje42 opened this issue Jan 2, 2024 · 12 comments
Open

Proposal: Possibility to not save all parameters. #1014

poje42 opened this issue Jan 2, 2024 · 12 comments
Assignees
Labels
automation Procedures, Experiment, and other automated things discussion-needed A solution still needs to be determined

Comments

@poje42
Copy link
Contributor

poje42 commented Jan 2, 2024

The idea is to have input parameters that are not saved as Parameters in the result files. I see at least two uses for this:

  1. To fit a GUI with many parameters nicely on the screen, the 'group_by'-parameter can be used. The parameter used to show the different groups needs not be saved.
  2. An ID of the DUT that the user enters and will be saved as metadata, as you don't want the parameter to be overwritten if you load parameters from a result file.

I've made a proposal in PR #1013 to add a Boolean 'save' parameter to the Parameter class and an example.
Maybe this is not the right way to go, I welcome better ideas!

@BenediktBurger
Copy link
Member

@msmttchr , @CasperSchippers , what do you think, as it touches what you mostly use?

@BenediktBurger BenediktBurger added automation Procedures, Experiment, and other automated things discussion-needed A solution still needs to be determined labels Jan 2, 2024
@CasperSchippers
Copy link
Collaborator

I think this is a good idea.

I do see a conflict between your two usecases with a single save boolean: you do not want to write the group-by parameter in your example to the file if you specify save=False; however, for the ID you do want to write it to file, but not load it upon using the parameters of the file. So that means that the save boolean has two different meanings, if I understand you correctly?

@CasperSchippers CasperSchippers self-assigned this Jan 2, 2024
@poje42
Copy link
Contributor Author

poje42 commented Jan 2, 2024

If you look in the example I've made you can see that I assign the ID parameter value to metadata (gui_no_save_param.py row 74) for use case 2.
I would say that use case 1 is "nice to have" and use case 2 is an actual issue I had.

Of course, it would be possible to add a metadata boolean to the Parameter class, but I saw this proposal as simpler and still make it possible to solve two use cases.

@CasperSchippers
Copy link
Collaborator

Ahh yes, missed that line; thanks for the explanation!

Due to the fact that I missed that, I actually started thinking that maybe explicit is better than implicit/obfuscated.
What I'm thinking now (and don't yet know how difficult/easy this would be) is that it could be most clear if the metadata class has some sort of "user input" toggle, or indeed a "metadata" toggle for a parameter; not sure what the best approach is het, so feel let me know what you think.

@poje42
Copy link
Contributor Author

poje42 commented Jan 4, 2024

Well, how to define "best"? :)
My initial thoughts was make it simple. The parameter already has the user interface (input fields, visibility control etc), types (float, list, integer etc), it was only missing that I don't want it to be overwritten. I can do any type of operation with input information before setting the metadata field(s) in startup(). Also, even if I don't see it today, a general input function may have other uses than those mentioned above.
As I understand your thoughts, the intention is to tighter connect metadata with the user input, which may be cleaner?

In that case I think we first should which structure we think is best:
Structure A

  1. Parameters (user input, load and save)
  2. Metadata (user input and save)

Structure B

  1. Parameters (user input, load and save)
  2. Metadata (save)
  3. Input values (user input)

@CasperSchippers
Copy link
Collaborator

CasperSchippers commented Jan 13, 2024

I understand (and also quite like) the thought of reusing the the parameters as you mention. In that sense I think there is also an option C we should not immediately discard:

Structure C:

  1. Parameters (user input, load & save, flag to disable loading)
  2. Metadata (save)

For structure A, if we want to have the different types of input field (float, list, etc), we might want to make separate classes from which both the Parameters and Metadata inherrit; downside is that I have feeling that this might end up in a lot of boilerplate code. The user-input should be optional

For structure B the same thought holds; I'm also a bit reluctant to add a seperate input values thing, as I think that the parameters and metadata should be able to catch the usecases.

So generally, for me options A and C would be the nicest, where A requires the most changes to the code (unless we just want a general text input field, instead of the int, float.. etc), and C the easiest to implement. What do you think?

@poje42
Copy link
Contributor Author

poje42 commented Jan 13, 2024

If I get you right, my initial propsal is close to structure C?

My initial thought was to have a flag that disables both save of a parameter and not touches the parameter at load (as the standard behaviour of a missing parameter is to be set to its default value).

Your propsal is to have a flag that only disable the loading of a parameter?

Both solutions will solve the use cases , but I think I prefer a flag that disable both save & load as I feel it more consistent: If a parameter is in the result file, it will be loaded.

What do you think?

@CasperSchippers
Copy link
Collaborator

My initial proposal was indeed close to structure C.

However, I gave it a bit more thought and realized a few things, and would slightly change my proposal at the moment to something that is actually closer to your structure B.

My current proposal is as follows (let's call it structure D for the sake of discussability):
It basically entails splitting the Parameter classes into a Parameter base-class and an InputField (maybe we'd need a better name) base-class, define a number of specified Input types that inherit from InputField (such as FloatInputField, IntInputField, basically the types of parameters we have now). Then we can have the specified Parameters inherit from both the specified input class and the Parameter base class; similarly we can have a Metadata base class and a set of specified Metadata classes. any of the Parameters and Metadata (sub)classes are written to file, but the InputField (subclasses) not, so you can also these as non-stored & non-loaded input fields.

The classes you'd have then would be (with the inheritance)
A few base-classes:

  1. InputField
  2. Parameter(InputField)
  3. Metadata(InputField)

A few type-specifications of the input class

  1. IntInputField(InputField)
  2. BooleanInputField(InputField)
  3. FloatInputField(InputField)
  4. VectorInputField(InputField)
  5. ListInputField(InputField)
  6. PhysicalInputField(InputField)

And the specified parameters and metadata (in number, only these are new, but the implementation will, I think, only require inheriting and nothing more):

  1. IntParameter(Parameter, IntInputField)
  2. BooleanParameter(Parameter, BooleanInputField)
  3. FloatParameter(Parameter, FloatInputField)
  4. VectorParameter(Parameter, VectorInputField)
  5. ListParameter(Parameter, ListInputField)
  6. PhysicalParameter(Parameter, PhysicalInputField)
  7. IntMetadata(Metadata, IntInputField)
  8. BooleanMetadata(Metadata, BooleanInputField)
  9. FloatMetadata(Metadata, FloatInputField)
  10. VectorMetadata(Metadata, VectorInputField)
  11. ListMetadata(Metadata, ListInputField)
  12. PhysicalMetadata(Metadata, PhysicalInputField)

I'm not yet entirely sure regarding whether the Parameter and Metadata base classes should already inherit of the InputField, or if we need an additional BaseMetadata and BaseParameter class. In my proposal, the specified parameters and metadata inherit from InputField twice; I'm not sure if this can cause issues, but it seems logical, and useful to me that they have a common ancestor. Also, I think it is closer to the current structure, as I'd like to keep the backwards compatibility such that people do not need to change things in their Procedures.

Additionally, we don't need a 'user-input' toggle for any of the classes, since we already have the 'inputs' keyword argument for the ManagedWindow.

What do you think of this architecture.

@poje42
Copy link
Contributor Author

poje42 commented Jan 16, 2024

That seems elegant.
Let me see if I got you right. Looking at it as a user:
xxxParameter() will work exactly as today.
Metadata() will work exactly as today (and as you have not set the 'inputs' argument, it will not be visible)
I.e. backward compatible
New features:
xxxMetadata() can have a user input by setting 'inputs' (and all other "input features" as Parameter has today)
xxxInputField() Also same as todays xxxParameter(), but not saved/loaded.
Is this correct?

It seems very clean. But as I have limited experience of Python and all structures of pymeasure, I don't see how complex it will be to implement. I guess that the basic handling of the input types that already is in todays Parameter can be reused, but it will of course require to be restructured. See any major risk in breaking some of todays behaviour?
Not sure I could managed such implementation, but I will be happy to assist.

@CasperSchippers
Copy link
Collaborator

Yeah, that sounds about right.

I can understand this seems a bit more difficult than the first implementation. I will indeed require a bit of restructuring, but I don't expect too many issues (though it will always be more than you think upfront). If you're uncomfortable with something like this, that is fine of course (not saying that is it necessary, but I could understand the reluctance), and I could give it a go (although, as you might've seen from my slow responding it might take a bit before it is finished).

@CasperSchippers
Copy link
Collaborator

CasperSchippers commented Jan 29, 2024

I opened a draft PR (#1036) in which I started working on an implementationo of the above idea.
I'm not yet entirely happy with the name InputField as the class from which both Parameter and Metadata inherrit, and which is also to be used direclty by users.

@poje42
Copy link
Contributor Author

poje42 commented Feb 1, 2024

Interesting, I'm looking forward to this. I'll take a look on it and see if I can learn some more.
What do see as problem with the name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation Procedures, Experiment, and other automated things discussion-needed A solution still needs to be determined
Projects
None yet
Development

No branches or pull requests

3 participants