Upgrade config #824
Upgrade config #824
Conversation
gonzaponte
force-pushed
the
upgrade-config
branch
2 times, most recently
from
5054001 to
359bb37
Compare
gonzaponte
force-pushed
the
upgrade-config
branch
from
359bb37 to
fbfa870
Compare
There was a problem hiding this comment.
Improves automatic verification of cities' parameters, resulting in better, enforced documentation and checking of configurations. An important step forward in long-term usability and mainainability of the code.
NB: this sits on top of #816, which needs merging first. #816 has already been approved.
| @@ -401,3 +416,240 @@ def test_configure_numpy(config_tmpdir): | |||
| conf_ns = configure(argv).as_namespace | |||
| assert hasattr(conf_ns, "a_numpy_array") | |||
| assert conf_ns.a_numpy_array.tolist() == [0, 0.5, 1] | |||
|
|
|||
|
|
|||
| @mark.parametrize( "value type".split() | |||
There was a problem hiding this comment.
While these tests are important, let's just take a few seconds to observe that we have over 100 lines of boilerplate that we get for free in a (strong! C++ doesn't cut the mustard) static type system!
| diffusion : Optional[Tuple[float, float, float]]=(1., 1., 0.3), | ||
| energy_type : Optional[HitEnergy]=HitEnergy.Ec, | ||
| deconv_mode : Optional[DeconvolutionMode]=DeconvolutionMode.joint, | ||
| n_dim : Optional[int]=2, | ||
| cut_type : Optional[CutType]=CutType.abs, | ||
| inter_method : Optional[InterpolationMethod]=InterpolationMethod.cubic, | ||
| n_iterations_g : Optional[int]=0): |
There was a problem hiding this comment.
Minor point: this does not conform to the IC alignment style. There are a few more instances of this in the code.
| , s1_nmin : int, s1_nmax : int | ||
| , s1_emin : float, s1_emax : float | ||
| , s1_wmin : float, s1_wmax : float | ||
| , s1_hmin : float, s1_hmax : float | ||
| , s1_ethr : float | ||
| , s2_nmin : int, s2_nmax : int | ||
| , s2_emin : float, s2_emax : float | ||
| , s2_wmin : float, s2_wmax : float | ||
| , s2_hmin : float, s2_hmax : float |
There was a problem hiding this comment.
I find the spatial proximity of the type of the earlier parameter to the name of the next parameter, a bit confusing to read. That is to say, in something like
, s1_nmin : float, s1_emax :floatthe natural inclination is to see the float, s1_emax as belonging together.
There was a problem hiding this comment.
I'm planning a PR applying a more uniform style everywhere, all these cosmetic changes will fit in there naturally.
Some default arguments have been removed ahead of the following commit
gonzaponte
force-pushed
the
upgrade-config
branch
from
fbfa870 to
f31b12c
Compare
This PR forces every city to be fully-annotate its input arguments. Hopefully, this will help new users to understand what parameters they need to provide to a city and slightly improve the self-documentation of the cities.
The types of input arguments are checked before running the city. Inner functions can also be fully-annotated and type-checked by using the provided decorator,
check_annotations.