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

Codify model restrictions and defaults #32

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

RastislavTuranyi
Copy link
Collaborator

Resolves #23

  • Structures the YAML data so that the restrictions and defaults are inside new dictionaries instead of using prefixes for names.
  • Makes it a requirement for each model to have the restrictions and defaults keys in its parameters dictionary (and therefore changes the YAML spec in Add YAML file specification #19).
  • Implements a generic (universal) validator for settings, though it is a bit clunky due to the need for passing dictionaries around (could be avoided by standardising the settings
  • Some of the tests had to be manhandled a little bit due to the changes in the error messages and validation order

@RastislavTuranyi RastislavTuranyi added the enhancement New feature or request label Feb 5, 2025
Copy link
Contributor

@oerc0122 oerc0122 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to over-ride/bypass restrictions for experimentation (without fiddling with the yaml)? E.g. optional arg


if isinstance(restriction, list):
if len(restriction) == 2:
if not restriction[0] <= setting <= restriction[1]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would I specify a restriction that is >0, but unbounded? Is there an easy way to specify inf?

Copy link
Collaborator Author

@RastislavTuranyi RastislavTuranyi Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val: [!!float inf] works for infinity

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a great example of "make the regular case easy and the exotic case possible".

If several models with Inf bounds come along, it could be worth special-casing. Until then, let's not.

Copy link
Contributor

@oerc0122 oerc0122 Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, just wondered whether it was possible given there may be properties that may be unbounded. There's also the common sense argument, that there doesn't have to be a restriction, but if you e.g. stick negative numbers in, expect strange results.

@RastislavTuranyi
Copy link
Collaborator Author

There isn't a simple way to bypass the restrictions (neither in this PR nor in the og implementation) - and in fact the discussion between raising an exception or using warnings has not been completely settled yet iirc - but if you are willing to be very involved you could construct the InstrumentModel directly (without using get_resolution_function), in which case you could intercept the data in the middle. That said, though, doing so wouldn't be easy at all since all our dataclasses are frozen=True to prevent tampering. Really, the intended way to mess with models is to create a new one, preferably by creating a custom YAML file (see the relevant how-to guide), but it might be worth revisiting some of these design choices, especially if you, effectively one of the end-users, would like such functionality. What do you think, @ajjackson?

@RastislavTuranyi
Copy link
Collaborator Author

RastislavTuranyi commented Feb 5, 2025

Actually, we have found that it is possible to mutate the Instrument._models dictionary in-place, so that would be an alternative way to dynamically edit the restrictions of a given model if you know what you're doing, though we agree with Adam that working with the YAML files is the recommended way.

EDIT: fyi: I have just discovered that yaml.safe_load handles YAML magic really smartly by using pointers as much as possible, which is good since it conserves memory, but it means that if you try doing the above, you might end up editing more bits than intended

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Codify model restrictions and defaults
3 participants