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

Standardize handling of default parameters in two sample tests #15

Open
morales-gregorio opened this issue Apr 1, 2020 · 3 comments
Open
Assignees
Milestone

Comments

@morales-gregorio
Copy link
Collaborator

I noticed that the test classes in tests/test_covariance_test.py and tests/test_power_spectrum_test.py differ quite a bit.

tests/test_power_spectrum_test.py uses a private method to set the default parameters. However, @rgutzen pointed out this could be done in the init function of the tow_sample_test abstract class. The default params would then be defined as a dictionary in the child classes, which would be very flexible and easy to understand for developers writing new tests.

Best,
Aitor

@rgerkin
Copy link

rgerkin commented Apr 1, 2020

SciUnit allows for parameters to be schematized, e.g. here. This schema is then inherited by all child classes, or can be modified piecewise in those classes. Setting params_schema then allows you to validate the parameters used in any instantiation of those classes, to make sure that all the required parameters are there, of the right type, within certain bounds, etc. That is handled in SciUnit for all tests here, by using a cerberus validator here.

@rgutzen rgutzen added this to the v0.2 milestone Apr 8, 2020
@rgutzen rgutzen self-assigned this Apr 8, 2020
@rgutzen
Copy link
Collaborator

rgutzen commented Apr 15, 2020

@rgerkin yes, params_schema is exactly what we were looking for. However, it seems that for checking the units of a parameter it is necessary to manually add a corresponding function and dict entry to the ParametersValidator class.
I feel there should be a general method to validate the units of a parameter by trying to rescale it to the unit specified in the schema (for example so that time parameters accept also 'ms' when 's' is specified). Although, looking at the cerberus source code I'm not sure how the Validator methods are actually executed.

@rgerkin
Copy link

rgerkin commented Apr 30, 2020

@rgutzen I wanted a general method but it is tricky because of the way that cerberus works and also that something simple like isinstance(x, pq.ms) doesn't work. isinstance can determine whether something is a python quantity, but not which quantity it is. Consequently there needs to be a custom validator which actually checks the unit type by doing quantities transformations.

I think it will rescale successfully though. On this line it simplifies the input units and checks for match, and simplifying in python quantities will turn e.g. ms into s so both 1000*ms and 1*s should both pass validation. So I think you need one function for each fundamental unit, but don't need additional ones for each prefix (milli, micro, etc.).

But if you figure out how to just make one general one that can handle all possible quantities, let me know!

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

No branches or pull requests

3 participants