-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: main
Are you sure you want to change the base?
Conversation
d566828
to
a82b036
Compare
c9de14c
to
f609b7e
Compare
059ae25
to
b336f14
Compare
There was a problem hiding this 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.
|
||
super().__setattr__(name, value) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 . . .
There was a problem hiding this comment.
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).
1277ce9
to
c1d1b7c
Compare
c1d1b7c
to
6eeab50
Compare
This PR fixes #99. It additionally:
test_examples.py
which ensures that all examples run without error (exceptconvert_rascal
, seeconvert_rascal
example doesn't work when called programmatically #102)I've compared the results of the new
DSPC_data_background
andDSPC_function_background
examples to those in the MATLAB and found that they give the same results between languages.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 #97ALL 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 #97The field contrastBackgroundTypes has been added to problemStructdone in Updates C++ and some unit tests #97