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

Updates backgrounds and resolutions in line with the MATLAB #100

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

Conversation

alexhroom
Copy link
Collaborator

@alexhroom alexhroom commented Dec 10, 2024

This PR fixes #99. It additionally:

I've compared the results of the new DSPC_data_background and DSPC_function_background examples to those in the MATLAB and found that they give the same results between languages.


  • The source field has been added to the backgrounds and resolutions models. This fields contains the background parameter (for constant backgrounds), data entry (for data backgrounds), or custom file entry (for function backgrounds). The value fields (i.e., value_1 to value_5) are not required for constant backgrounds, value_1 contains an optional background_parameter used as a data offset, or contain up to five function arguments (all defined as background_parameters).
  • For resolutions, the function type is still not supported, constant works in the same way as for backgrounds, but for data, neither the source nor value fields are required.
  • When the background/resolution type is changed, the source and value fields should be cleared.
  • In the examples and jupyter notebooks, value_1 needs to be changed to source for constant type backgrounds and resolutions.
  • Use a python version of the data and function background examples to check everything works (probably worth adding them to the examples too).
  • The check_indices routine needs to be modified to reflect the change of contrastBackgroundParams to be a nested list, with a variable number of inputs, and to consider a custom file entry in the case of function backgrounds.
  • A routine mirroring insertDataBackgroundIntoContrastData is required, to populate the additional columns in the data files for data backgrounds.
  • The output result has had backgroundParams removed from the contrastParams sub struct, with backgrounds added to the main result struct. done in Updates C++ and some unit tests #97
  • ALL data files in the input problemStruct have been extended to EXACTLY six columns. Any empty columns are filled with zero values. done in Updates C++ and some unit tests #97
  • The problemStruct input contrastBackgroundParams has been modified to be a nested list. The nested lists may contain between one and five indices. These are the indices of the source field (from the appropriate data struct), and from the values fields: nothing else (for constant backgrounds), a potential offset (value_1 if non-empty, for data backgrounds), or up to five function arguments (for function backgrounds).
  • The field contrastBackgroundTypes has been added to problemStruct done in Updates C++ and some unit tests #97

@alexhroom alexhroom force-pushed the 99-background-types branch 7 times, most recently from d566828 to a82b036 Compare December 10, 2024 11:55
@alexhroom alexhroom changed the title Update backgrounds and resolutions in line with the MATLAB Updates backgrounds and resolutions in line with the MATLAB Dec 10, 2024
Copy link
Collaborator

@DrPaulSharp DrPaulSharp left a comment

Choose a reason for hiding this comment

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

This is looking good so far. Alongside the comments, please add "backgrounds" to the output results dataclass (it's available in the C++), and then to the tests. As discussed, we should look to add the other updates to the inputs and outputs into the tests in another PR.

RATapi/inputs.py Show resolved Hide resolved
RATapi/inputs.py Show resolved Hide resolved
RATapi/inputs.py Outdated Show resolved Hide resolved
RATapi/models.py Outdated Show resolved Hide resolved

super().__setattr__(name, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two things here. The matlab raises a warning when fields are cleared, so we should do the same here. Also, when I tried to change the type of a background via set_fields, I got a validation error saying the contents of the source field should be defined in data, so the field was not cleared prior to revalidation of the project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not too sure what to do with that - ClassList.set_fields essentially creates a new object and puts it in the index of the old one, so no attributes actually 'get set'. should I refactor ClassList.set_fields to go through the proper assignment methods instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that refactor would cause it's own problems I think. If I recall correctly that's why I moved in that direction. You might have to write a validator that's something like set_contrast_model_field in the project and keep the "old" type as a hidden variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's right, I had set_fields to loop through the fields set and apply them, but ran into problems with parameters. If you change min, max and value the order in which they're applied mean that you can run into intermediate validation errors.
Is there a refactor to set_fields that could avoid this? Should we try to supress validation errors whilst we update multiple field values and revalidate the model at the end?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You'd want to supress errors raised by "after" model validators, but not necessarily those from "before" model validators or field validators. Hmmm . . .

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I'm thinking might be the ideal solution is to somehow turn off validate_assignment within set_fields (presumerably via a context manager) and then validate the model at the end, so any problems are caught. But then maybe biting the bullet and tuning off validation errors will work essentially as well (and definitely will be easier).

RATapi/models.py Outdated Show resolved Hide resolved
RATapi/models.py Show resolved Hide resolved
RATapi/models.py Show resolved Hide resolved
RATapi/models.py Outdated Show resolved Hide resolved
@alexhroom alexhroom force-pushed the 99-background-types branch 3 times, most recently from 1277ce9 to c1d1b7c Compare December 16, 2024 12:40
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.

Update backgrounds and resolutions in line with the MATLAB
2 participants