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

Added Sanity checks, to ensure congruent Settings. #50

Closed
wants to merge 11 commits into from

Conversation

appetrosyan
Copy link
Contributor

Meant to solve #47. May need testing, but instead of catching errors and raising errors early, this produces a warning (for now, it's trivial to change to raise ValueError).

I'm not quite sure about the convention of using negative numbers to set defaults. This is not pythonic. The justification is that this is the convention in other languages, and when interoperating, it's easier to maintain only one version. However, we're already providing the defaults in the doctoring, so we need to duplicate the effort anyway. It may be better to add another module that contains the defaults, and update it.

Copy link
Member

@williamjameshandley williamjameshandley left a comment

Choose a reason for hiding this comment

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

Hi @appetrosyan, I very much approve of providing further warnings/exceptions to users providing invalid arguments, so I think these should definitely be brought in, and the nDims and nDerived arguments are pretty useful additions too.

I'm concerned that some of your changes may not be python2 compatible, and the best way to test this is to re-push with some minor change (say removing the "this is the old way we did it" block) as I've now fixed travis since it's migration from org to com.

My main concern with this is that while I agree your new way of dealing with arguments is definitely neater and more compact, it is a potentially backward-incompatible shift from kwargs to args. How much does the checking depend on this slicker interface, and are we able to keep the clunkier original kwargs approach for now?

@appetrosyan
Copy link
Contributor Author

Ok. I specifically checked if it's python2 compatible (otherwise the Python3 way is using a @dataclass object). Pushing now!

@appetrosyan
Copy link
Contributor Author

About the shift from *args/**kwargs to explicit keyword arguments. It's not a breaking change. The opposite would be though.

Python dictionaries are unordered by default, meaning that **kwargs can't be interpreted as positional arguments. By introducing the keyword arguments in the signature, we allow them being passed in the order in which they are defined.

So by definition every configuration that used to work with the old way, should work now too.

An alternative (yet also neat way) that preserves the **kwargs style would be to create a dictionary of default values. This will not broaden the scope of acceptable input, but will have the benefits of clarity and conciseness (cobaya does it in a similar way).

I have considered other approaches, e.g. @dataclass with slots, as per here, but that would definitely break python2 compatibility.

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.

2 participants